On Sun, Sep 04, 2016 at 05:43:20PM +0200, Martin Pieuchot wrote:
> Thanks to awolk@ hacking on USB wireless we found a lot of new races in
> the network stack.
>
> Passing and ``ifp'' pointer to a task is *not* safe. If the task
> sleeps, then another thread my start executing if_detach() freeing the
> memory associated to ``ifp''. And task do sleep, that's why we use
> them in the first place.
>
> So we should always pass and ifp index like I did with the
> if_input_process() task. Diff attached fix awolk@'s first panic.
>
> ok?
>
OK claudio@
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.441
> diff -u -p -r1.441 if.c
> --- net/if.c 4 Sep 2016 10:32:01 -0000 1.441
> +++ net/if.c 4 Sep 2016 11:03:37 -0000
> @@ -400,6 +400,8 @@ if_map_dtor(void *null, void *m)
> void
> if_attachsetup(struct ifnet *ifp)
> {
> + unsigned long ifidx;
> +
> TAILQ_INIT(&ifp->if_groups);
>
> if_addgroup(ifp, IFG_ALL);
> @@ -409,18 +411,19 @@ if_attachsetup(struct ifnet *ifp)
> pfi_attach_ifnet(ifp);
> #endif
>
> - task_set(ifp->if_watchdogtask, if_watchdog_task, ifp);
> timeout_set(ifp->if_slowtimo, if_slowtimo, ifp);
> if_slowtimo(ifp);
>
> - task_set(ifp->if_linkstatetask, if_linkstate, ifp);
> -
> if_idxmap_insert(ifp);
> KASSERT(if_get(0) == NULL);
>
> + ifidx = ifp->if_index;
> +
> mq_init(&ifp->if_inputqueue, 8192, IPL_NET);
> - task_set(ifp->if_inputtask, if_input_process,
> - (void *)(unsigned long)ifp->if_index);
> + task_set(ifp->if_inputtask, if_input_process, (void *)ifidx);
> + task_set(ifp->if_watchdogtask, if_watchdog_task, (void *)ifidx);
> + task_set(ifp->if_linkstatetask, if_linkstate, (void *)ifidx);
> +
>
> /* Announce the interface. */
> rt_ifannouncemsg(ifp, IFAN_ARRIVAL);
> @@ -1482,11 +1485,16 @@ if_up(struct ifnet *ifp)
> * a link-state transition.
> */
> void
> -if_linkstate(void *xifp)
> +if_linkstate(void *xifidx)
> {
> - struct ifnet *ifp = xifp;
> + unsigned int ifidx = (unsigned long)xifidx;
> + struct ifnet *ifp;
> int s;
>
> + ifp = if_get(ifidx);
> + if (ifp == NULL)
> + return;
> +
> s = splsoftnet();
> rt_ifmsg(ifp);
> #ifndef SMALL_KERNEL
> @@ -1494,6 +1502,8 @@ if_linkstate(void *xifp)
> #endif
> dohooks(ifp->if_linkstatehooks, 0);
> splx(s);
> +
> + if_put(ifp);
> }
>
> /*
> @@ -1525,15 +1535,22 @@ if_slowtimo(void *arg)
> }
>
> void
> -if_watchdog_task(void *arg)
> +if_watchdog_task(void *xifidx)
> {
> - struct ifnet *ifp = arg;
> + unsigned int ifidx = (unsigned long)xifidx;
> + struct ifnet *ifp;
> int s;
>
> + ifp = if_get(ifidx);
> + if (ifp == NULL)
> + return;
> +
> s = splnet();
> if (ifp->if_watchdog)
> (*ifp->if_watchdog)(ifp);
> splx(s);
> +
> + if_put(ifp);
> }
>
> /*
--
:wq Claudio
