On Fri, Nov 15, 2013 at 03:20:48PM +0100, Mike Belopuhov wrote:
> On 15 November 2013 15:13, Stefan Sperling <s...@openbsd.org> wrote:
> > Is this done right?
> >
> > Works here with pppoe(4) for both IPv4 and IPv6.
> >
>
> i think this diff might lack task_del's in the detach code.
Ooops, good catch.
> have you tried destroying your pppoe interface?
Yes, but evidently not while a task was scheduled.
Same diff with task_del calls added.
Index: if_sppp.h
===================================================================
RCS file: /cvs/src/sys/net/if_sppp.h,v
retrieving revision 1.19
diff -u -p -r1.19 if_sppp.h
--- if_sppp.h 14 Nov 2013 16:52:33 -0000 1.19
+++ if_sppp.h 15 Nov 2013 13:48:47 -0000
@@ -93,6 +93,12 @@ struct spppreq {
#ifdef _KERNEL
#include <sys/timeout.h>
+#include <sys/task.h>
+
+#ifdef INET6
+#include <netinet/in.h>
+#include <netinet6/in6_var.h>
+#endif
#define IDX_LCP 0 /* idx into state table */
@@ -124,8 +130,13 @@ struct sipcp {
#define IPV6CP_MYIFID_SEEN 2 /* have seen my suggested ifid */
u_int32_t saved_hisaddr; /* if hisaddr (IPv4) is dynamic, save
* original one here, in network byte order */
- u_int32_t req_hisaddr; /* remote address requested */
- u_int32_t req_myaddr; /* local address requested */
+ u_int32_t req_hisaddr; /* remote address requested (IPv4) */
+ u_int32_t req_myaddr; /* local address requested (IPv4) */
+#ifdef INET6
+ struct in6_aliasreq req_ifid; /* local ifid requested (IPv6) */
+#endif
+ struct task set_addr_task; /* set address from process context */
+ struct task clear_addr_task; /* clear address from process context */
};
struct sauth {
Index: if_spppsubr.c
===================================================================
RCS file: /cvs/src/sys/net/if_spppsubr.c,v
retrieving revision 1.112
diff -u -p -r1.112 if_spppsubr.c
--- if_spppsubr.c 14 Nov 2013 16:52:33 -0000 1.112
+++ if_spppsubr.c 15 Nov 2013 14:37:24 -0000
@@ -46,7 +46,6 @@
#include <sys/syslog.h>
#include <sys/malloc.h>
#include <sys/mbuf.h>
-#include <sys/workq.h>
#include <sys/timeout.h>
#include <crypto/md5.h>
@@ -72,10 +71,6 @@
#include <net/if_sppp.h>
-#ifdef INET6
-#include <netinet6/in6_var.h>
-#endif
-
# define UNTIMEOUT(fun, arg, handle) \
timeout_del(&(handle))
@@ -291,6 +286,7 @@ HIDE void sppp_lcp_check_and_close(struc
HIDE int sppp_ncp_check(struct sppp *sp);
HIDE void sppp_ipcp_init(struct sppp *sp);
+HIDE void sppp_ipcp_destroy(struct sppp *sp);
HIDE void sppp_ipcp_up(struct sppp *sp);
HIDE void sppp_ipcp_down(struct sppp *sp);
HIDE void sppp_ipcp_open(struct sppp *sp);
@@ -306,6 +302,7 @@ HIDE void sppp_ipcp_tlf(struct sppp *sp)
HIDE void sppp_ipcp_scr(struct sppp *sp);
HIDE void sppp_ipv6cp_init(struct sppp *sp);
+HIDE void sppp_ipv6cp_destroy(struct sppp *sp);
HIDE void sppp_ipv6cp_up(struct sppp *sp);
HIDE void sppp_ipv6cp_down(struct sppp *sp);
HIDE void sppp_ipv6cp_open(struct sppp *sp);
@@ -902,6 +899,9 @@ sppp_detach(struct ifnet *ifp)
struct sppp **q, *p, *sp = (struct sppp*) ifp;
int i;
+ sppp_ipcp_destroy(sp);
+ sppp_ipv6cp_destroy(sp);
+
/* Remove the entry from the keepalive list. */
for (q = &spppq; (p = *q); q = &p->pp_next)
if (p == sp) {
@@ -2637,6 +2637,15 @@ sppp_ipcp_init(struct sppp *sp)
sp->ipcp.flags = 0;
sp->state[IDX_IPCP] = STATE_INITIAL;
sp->fail_counter[IDX_IPCP] = 0;
+ task_set(&sp->ipcp.set_addr_task, sppp_set_ip_addrs, sp, NULL);
+ task_set(&sp->ipcp.clear_addr_task, sppp_clear_ip_addrs, sp, NULL);
+}
+
+HIDE void
+sppp_ipcp_destroy(struct sppp *sp)
+{
+ task_del(systq, &sp->ipcp.set_addr_task);
+ task_del(systq, &sp->ipcp.clear_addr_task);
}
HIDE void
@@ -2955,38 +2964,11 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struc
addlog("\n");
}
-struct sppp_set_ip_addrs_args {
- struct sppp *sp;
- u_int32_t myaddr;
- u_int32_t hisaddr;
-};
-
HIDE void
sppp_ipcp_tlu(struct sppp *sp)
{
- struct ifnet *ifp = &sp->pp_if;
- struct sppp_set_ip_addrs_args *args;
-
- args = malloc(sizeof(*args), M_TEMP, M_NOWAIT);
- if (args == NULL)
- return;
-
- args->sp = sp;
-
- /* we are up. Set addresses and notify anyone interested */
- sppp_get_ip_addrs(sp, &args->myaddr, &args->hisaddr, 0);
- if ((sp->ipcp.flags & IPCP_MYADDR_DYN) &&
- (sp->ipcp.flags & IPCP_MYADDR_SEEN))
- args->myaddr = sp->ipcp.req_myaddr;
- if ((sp->ipcp.flags & IPCP_HISADDR_DYN) &&
- (sp->ipcp.flags & IPCP_HISADDR_SEEN))
- args->hisaddr = sp->ipcp.req_hisaddr;
-
- if (workq_add_task(NULL, 0, sppp_set_ip_addrs, args, NULL)) {
- free(args, M_TEMP);
- printf("%s: workq_add_task failed, cannot set "
- "addresses\n", ifp->if_xname);
- }
+ if (sp->ipcp.req_myaddr != 0 || sp->ipcp.req_hisaddr != 0)
+ task_add(systq, &sp->ipcp.set_addr_task);
}
HIDE void
@@ -3043,15 +3025,9 @@ sppp_ipcp_tls(struct sppp *sp)
HIDE void
sppp_ipcp_tlf(struct sppp *sp)
{
- struct ifnet *ifp = &sp->pp_if;
-
if (sp->ipcp.flags & (IPCP_MYADDR_DYN|IPCP_HISADDR_DYN))
/* Some address was dynamic, clear it again. */
- if (workq_add_task(NULL, 0,
- sppp_clear_ip_addrs, (void *)sp, NULL)) {
- printf("%s: workq_add_task failed, cannot clear "
- "addresses\n", ifp->if_xname);
- }
+ task_add(systq, &sp->ipcp.clear_addr_task);
/* we no longer need LCP */
sp->lcp.protos &= ~(1 << IDX_IPCP);
@@ -3110,6 +3086,14 @@ sppp_ipv6cp_init(struct sppp *sp)
sp->ipv6cp.flags = 0;
sp->state[IDX_IPV6CP] = STATE_INITIAL;
sp->fail_counter[IDX_IPV6CP] = 0;
+ task_set(&sp->ipv6cp.set_addr_task, sppp_update_ip6_addr, sp,
+ &sp->ipv6cp.req_ifid);
+}
+
+HIDE void
+sppp_ipv6cp_destroy(struct sppp *sp)
+{
+ task_del(systq, &sp->ipv6cp.set_addr_task);
}
HIDE void
@@ -3498,6 +3482,11 @@ sppp_ipv6cp_init(struct sppp *sp)
}
HIDE void
+sppp_ipv6cp_destroy(struct sppp *sp)
+{
+}
+
+HIDE void
sppp_ipv6cp_up(struct sppp *sp)
{
}
@@ -4587,16 +4576,15 @@ sppp_update_gw(struct ifnet *ifp)
}
/*
- * Work queue task adding addresses from process context.
+ * Task adding addresses from process context.
* If an address is 0, leave it the way it is.
*/
HIDE void
sppp_set_ip_addrs(void *arg1, void *arg2)
{
- struct sppp_set_ip_addrs_args *args = arg1;
- struct sppp *sp = args->sp;
- u_int32_t myaddr = args->myaddr;
- u_int32_t hisaddr = args->hisaddr;
+ struct sppp *sp = arg1;
+ u_int32_t myaddr;
+ u_int32_t hisaddr;
struct ifnet *ifp = &sp->pp_if;
int debug = ifp->if_flags & IFF_DEBUG;
struct ifaddr *ifa;
@@ -4604,8 +4592,13 @@ sppp_set_ip_addrs(void *arg1, void *arg2
struct sockaddr_in *dest;
int s;
- /* Arguments are now on local stack so free temporary storage. */
- free(args, M_TEMP);
+ sppp_get_ip_addrs(sp, &myaddr, &hisaddr, NULL);
+ if ((sp->ipcp.flags & IPCP_MYADDR_DYN) &&
+ (sp->ipcp.flags & IPCP_MYADDR_SEEN))
+ myaddr = sp->ipcp.req_myaddr;
+ if ((sp->ipcp.flags & IPCP_HISADDR_DYN) &&
+ (sp->ipcp.flags & IPCP_HISADDR_SEEN))
+ hisaddr = sp->ipcp.req_hisaddr;
s = splsoftnet();
@@ -4656,7 +4649,7 @@ sppp_set_ip_addrs(void *arg1, void *arg2
}
/*
- * Work queue task clearing addresses from process context.
+ * Task clearing addresses from process context.
* Clear IP addresses.
*/
HIDE void
@@ -4772,7 +4765,6 @@ sppp_update_ip6_addr(void *arg1, void *a
if (ia == NULL) {
/* IPv6 disabled? */
splx(s);
- free(ifra, M_TEMP);
return;
}
@@ -4791,7 +4783,6 @@ sppp_update_ip6_addr(void *arg1, void *a
SPP_ARGS(ifp), error);
}
splx(s);
- free(ifra, M_TEMP);
}
/*
@@ -4802,12 +4793,9 @@ sppp_set_ip6_addr(struct sppp *sp, const
const struct in6_addr *dst)
{
struct ifnet *ifp = &sp->pp_if;
- struct in6_aliasreq *ifra;
-
- ifra = malloc(sizeof(*ifra), M_TEMP, M_NOWAIT|M_ZERO);
- if (ifra == NULL)
- return;
+ struct in6_aliasreq *ifra = &sp->ipv6cp.req_ifid;
+ bzero(ifra, sizeof(*ifra));
bcopy(ifp->if_xname, ifra->ifra_name, sizeof(ifra->ifra_name));
ifra->ifra_addr.sin6_len = sizeof(struct sockaddr_in6);
@@ -4831,11 +4819,7 @@ sppp_set_ip6_addr(struct sppp *sp, const
/* DAD is redundant after an IPv6CP exchange. */
ifra->ifra_flags |= IN6_IFF_NODAD;
- if (workq_add_task(NULL, 0, sppp_update_ip6_addr, sp, ifra)) {
- free(ifra, M_TEMP);
- printf("%s: workq_add_task failed, cannot set IPv6 "
- "addresses\n", ifp->if_xname);
- }
+ task_add(systq, &sp->ipv6cp.set_addr_task);
}
/*
