On 06/27/12 10:59, RD Thrush wrote: > On 06/26/12 12:36, Matthew Dempsky wrote: >> This looks similar to the pppoe(4) bug that stsp fixed last year: >> http://marc.info/?l=openbsd-tech&m=130288210121749&w=2 >> >> Seems to me like sppp_clear_ip_addresses() needs the same workq >> treatment that sppp_set_ip_addresses() received. > > Thanks for the heads up. I made the attached patch and have been running with > it for the past 3 hours. So far, it has successfully gone through 7 > disconnects > without the stack trace diagnostic. I'll report more later unless breakage > occurs.
Status update: While running the patch, my DSL continued to fail in the same way for another day or so until full service was regained. During that time, it handled 30+ disconnections without triggering the stack trace diagnostic. I'm confident the patch fixed my problem and may be considered for the tree. Thanks, Bob I've appended the patch inline or [1] if tbird mangles it. [1]<http://arp.thrush.com/openbsd/pppoe/sys_net_if_spppsubr_c.diff> Give sppp_clear_ip_addresses() the same workq treatment that sppp_set_ip_addresses() received in cvs r1.85 by stsp. Index: sys/net/if_spppsubr.c =================================================================== RCS file: /a8v/pub2/cvsroot/OpenBSD/src/sys/net/if_spppsubr.c,v retrieving revision 1.96 diff -u -p -u -p -r1.96 if_spppsubr.c --- sys/net/if_spppsubr.c 28 Jan 2012 12:14:45 -0000 1.96 +++ sys/net/if_spppsubr.c 27 Jun 2012 11:35:01 -0000 @@ -398,7 +398,7 @@ HIDE void sppp_qflush(struct ifqueue *if int sppp_update_gw_walker(struct radix_node *rn, void *arg, u_int); void sppp_update_gw(struct ifnet *ifp); HIDE void sppp_set_ip_addrs(void *, void *); -HIDE void sppp_clear_ip_addrs(struct sppp *sp); +HIDE void sppp_clear_ip_addrs(void *, void *); HIDE void sppp_set_phase(struct sppp *sp); /* our control protocol descriptors */ @@ -3014,6 +3014,10 @@ struct sppp_set_ip_addrs_args { u_int32_t hisaddr; }; +struct sppp_clear_ip_addrs_args { + struct sppp *sp; +}; + HIDE void sppp_ipcp_tlu(struct sppp *sp) { @@ -3035,6 +3039,7 @@ sppp_ipcp_tlu(struct sppp *sp) (sp->ipcp.flags & IPCP_HISADDR_SEEN)) args->hisaddr = sp->ipcp.req_hisaddr; + printf("%s: workq_add_task(sppp_set_ip_addrs)\n", ifp->if_xname); /* XXX */ if (workq_add_task(NULL, 0, sppp_set_ip_addrs, args, NULL)) { free(args, M_TEMP); printf("%s: workq_add_task failed, cannot set " @@ -3096,9 +3101,23 @@ sppp_ipcp_tls(struct sppp *sp) HIDE void sppp_ipcp_tlf(struct sppp *sp) { + struct ifnet *ifp = &sp->pp_if; + struct sppp_clear_ip_addrs_args *args; + + args = malloc(sizeof(*args), M_TEMP, M_NOWAIT); + if (args == NULL) + return; + + args->sp = sp; + if (sp->ipcp.flags & (IPCP_MYADDR_DYN|IPCP_HISADDR_DYN)) /* Some address was dynamic, clear it again. */ - sppp_clear_ip_addrs(sp); + printf("%s: workq_add_task(sppp_clear_ip_addrs)\n", ifp->if_xname); /* XXX */ + if (workq_add_task(NULL, 0, sppp_clear_ip_addrs, args, NULL)) { + free(args, M_TEMP); + printf("%s: workq_add_task failed, cannot clear " + "addresses\n", ifp->if_xname); + } /* we no longer need LCP */ sp->lcp.protos &= ~(1 << IDX_IPCP); @@ -4755,21 +4774,33 @@ sppp_set_ip_addrs(void *arg1, void *arg2 return; } sppp_update_gw(ifp); + log(LOG_DEBUG, SPP_FMT "sppp_set_ip_addrs succeeded\n", SPP_ARGS(ifp)); /* XXX */ } splx(s); } /* - * Clear IP addresses. Must be called at splnet. + * Work queue task clearing addresses from process context. + * Clear IP addresses. */ HIDE void -sppp_clear_ip_addrs(struct sppp *sp) +sppp_clear_ip_addrs(void *arg1, void *arg2) { + struct sppp_clear_ip_addrs_args *args = arg1; + struct sppp *sp = args->sp; struct ifnet *ifp = &sp->pp_if; + int debug = ifp->if_flags & IFF_DEBUG; struct ifaddr *ifa; struct sockaddr_in *si; struct sockaddr_in *dest; + int s; + + /* Arguments are now on local stack so free temporary storage. */ + free(args, M_TEMP); + + s = splsoftnet(); + u_int32_t remote; if (sp->ipcp.flags & IPCP_HISADDR_DYN) remote = sp->ipcp.saved_hisaddr; @@ -4792,6 +4823,7 @@ sppp_clear_ip_addrs(struct sppp *sp) } if (ifa && si) { + int error; struct sockaddr_in new_sin = *si; in_ifscrub(ifp, ifatoia(ifa)); @@ -4800,10 +4832,18 @@ sppp_clear_ip_addrs(struct sppp *sp) if (sp->ipcp.flags & IPCP_HISADDR_DYN) /* replace peer addr in place */ dest->sin_addr.s_addr = sp->ipcp.saved_hisaddr; - if (!in_ifinit(ifp, ifatoia(ifa), &new_sin, 0, 0)) + if (!(error = in_ifinit(ifp, ifatoia(ifa), &new_sin, 0, 0))) dohooks(ifp->if_addrhooks, 0); + if (debug && error) { + log(LOG_DEBUG, SPP_FMT "sppp_clear_ip_addrs: in_ifinit " + " failed, error=%d\n", SPP_ARGS(ifp), error); + splx(s); + return; + } sppp_update_gw(ifp); + log(LOG_DEBUG, SPP_FMT "sppp_clear_ip_addrs succeeded\n", SPP_ARGS(ifp)); /* XXX */ } + splx(s); }
