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.
Index: src/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
--- src/sys/net/if_spppsubr.c 28 Jan 2012 12:14:45 -0000 1.96
+++ src/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);
}
