On 15/11/13(Fri) 15:45, Stefan Sperling wrote:
> 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.
Even if right now calling task_del() is enough, do you know if there's
an easy way to convert this code without putting the task storage in
the chunk of memory it manipulates? In other words having the "struct
task" outside of the softc?
I'm asking because if you can do it, this will make the task totally
independent and won't require any task_del(). The idea behind that
is that tomorrow we will try to have more kernel threads running in
parallel, and if your task is about to run or already running when
your interface is destroyed you might end up freeing the memory the
task is manipulating.
Since the current code is already using allocating some memory for the
arguments of the task, I'd argue that this is better than putting the
task storage on the same memory chunk that it manipulates. But since
this problem exists in other places in our tree, we might think of an
alternative solution/api/whatever.
Other than that your diff looks ok.
> 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);
> }
>
> /*
>
