On 30/10/13(Wed) 16:48, Alexander Bluhm wrote:
> On Fri, Oct 18, 2013 at 01:00:25PM +0200, Martin Pieuchot wrote:
> > On 18/10/13(Fri) 12:45, Alexander Bluhm wrote:
> > >
> > > Ethernet drivers connected via USB might sleep when their multicast
> > > group filter is modified. Unfortunately this happens from softclock
> > > or softnet interrupt when IPv6 decides to unconfigure its addresses
> > > automatically.
> > >
> > > An obvious solution is to use a work queue. I have put the workq
> > > storage into struct in6_multi_mship. That requires to include
> > > sys/workq.h before compiling this struct.
> >
> > One problem with a work queue is that you cannot guarantee the interface
> > will still be here when the task will be scheduled, so passing a pointer
> > might not be a good idea.
> >
> > That is why I wanted to change the if_get() API to use unique index for
> > each interface...
> >
> > But maybe there's another solution at this problem than using a workq...
>
> Now I use the if_index to detect that the interface is gone.
>
> I this a better approach?
Do you know if the memory pointed by the "imm" pointer you're passing to
your workq can be freed before the task got scheduled, if the interface
is removed/destroyed for example?
Because the whole idea of passing a token/id, the interface index in our
case, only works because we are sure the array of interface pointers
will never be freed.
There's also a risk of calling in6_leavegroup() a second time before the
first task get scheduled, in this case the second task will have a bogus
"imm" pointer. This problem can be avoided by using a task(9) though.
I'm not sure to get the whole picture about this ioctl called in an
interrupt context so feel free to correct me, but the only offending
function is nd6_timer(), is that right?
Since nd6_timer() iterates over global structures I think the easier
option would be to defer its execution to a task. I don't think this
is a problem since the nd6_prune timer is rather big (1 sec by default)
and I'd guess this solution is less intrusive, if of course that's the
only offending path ;)
Finally since we are moving away from workq(9) to task(9) I'd suggest
you to use the latter.
Martin
>
> bluhm
>
> Index: netinet6/in6.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6.c,v
> retrieving revision 1.122
> diff -u -p -u -p -r1.122 in6.c
> --- netinet6/in6.c 24 Oct 2013 11:20:18 -0000 1.122
> +++ netinet6/in6.c 30 Oct 2013 15:01:20 -0000
> @@ -124,6 +124,7 @@ int in6_lifaddr_ioctl(struct socket *, u
> int in6_ifinit(struct ifnet *, struct in6_ifaddr *, int);
> void in6_unlink_ifa(struct in6_ifaddr *, struct ifnet *);
> void in6_ifloop_request(int, struct ifaddr *);
> +void in6_leavegroup_task(void *, void *);
>
> const struct sockaddr_in6 sa6_any = {
> sizeof(sa6_any), AF_INET6, 0, 0, IN6ADDR_ANY_INIT, 0
> @@ -1777,16 +1778,18 @@ in6_delmulti(struct in6_multi *in6m)
> ifafree(&in6m->in6m_ia->ia_ifa); /* release reference */
> }
>
> - /*
> - * Notify the network driver to update its multicast
> - * reception filter.
> - */
> - bzero(&ifr.ifr_addr, sizeof(struct sockaddr_in6));
> - ifr.ifr_addr.sin6_len = sizeof(struct sockaddr_in6);
> - ifr.ifr_addr.sin6_family = AF_INET6;
> - ifr.ifr_addr.sin6_addr = in6m->in6m_addr;
> - (*in6m->in6m_ifp->if_ioctl)(in6m->in6m_ifp,
> - SIOCDELMULTI, (caddr_t)&ifr);
> + if (in6m->in6m_ifp != NULL) {
> + /*
> + * Notify the network driver to update its multicast
> + * reception filter.
> + */
> + bzero(&ifr.ifr_addr, sizeof(struct sockaddr_in6));
> + ifr.ifr_addr.sin6_len = sizeof(struct sockaddr_in6);
> + ifr.ifr_addr.sin6_family = AF_INET6;
> + ifr.ifr_addr.sin6_addr = in6m->in6m_addr;
> + (*in6m->in6m_ifp->if_ioctl)(in6m->in6m_ifp,
> + SIOCDELMULTI, (caddr_t)&ifr);
> + }
> free(in6m, M_IPMADDR);
> }
> splx(s);
> @@ -1814,11 +1817,26 @@ in6_joingroup(struct ifnet *ifp, struct
> int
> in6_leavegroup(struct in6_multi_mship *imm)
> {
> -
> if (imm->i6mm_maddr)
> - in6_delmulti(imm->i6mm_maddr);
> - free(imm, M_IPMADDR);
> + workq_queue_task(NULL, &imm->wqt, 0, in6_leavegroup_task, imm,
> + (void *)(unsigned long)imm->i6mm_maddr->in6m_ifp->if_index);
> + else
> + free(imm, M_IPMADDR);
> return 0;
> +}
> +
> +void
> +in6_leavegroup_task(void *arg1, void *arg2)
> +{
> + struct in6_multi_mship *imm = arg1;
> + unsigned int index = (unsigned long)arg2;
> +
> + /* If interface has been be freed, avoid call to if_ioctl(). */
> + if (if_get(index) != imm->i6mm_maddr->in6m_ifp)
> + imm->i6mm_maddr->in6m_ifp = NULL;
> +
> + in6_delmulti(imm->i6mm_maddr);
> + free(imm, M_IPMADDR);
> }
>
> /*
> Index: netinet6/in6_var.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/in6_var.h,v
> retrieving revision 1.44
> diff -u -p -u -p -r1.44 in6_var.h
> --- netinet6/in6_var.h 24 Oct 2013 11:31:43 -0000 1.44
> +++ netinet6/in6_var.h 30 Oct 2013 15:01:20 -0000
> @@ -64,6 +64,8 @@
> #ifndef _NETINET6_IN6_VAR_H_
> #define _NETINET6_IN6_VAR_H_
>
> +#include <sys/workq.h>
> +
> /*
> * Interface address, Internet version. One of these structures
> * is allocated for each interface with an Internet address.
> @@ -480,6 +482,7 @@ do {
> \
> * belongs to.
> */
> struct in6_multi_mship {
> + struct workq_task wqt; /* Allow network driver to sleep */
> struct in6_multi *i6mm_maddr; /* Multicast address pointer */
> LIST_ENTRY(in6_multi_mship) i6mm_chain; /* multicast options chain */
> };
>
