On 16/06/21(Wed) 14:26, David Gwynne wrote:
>
>
> > On 16 Jun 2021, at 00:39, Martin Pieuchot <[email protected]> wrote:
> >
> > On 15/06/21(Tue) 22:52, David Gwynne wrote:
> >> On Mon, Jun 14, 2021 at 10:07:58AM +0200, Martin Pieuchot wrote:
> >>> On 10/06/21(Thu) 19:17, Alexander Bluhm wrote:
> >> [...]
> >>>> The in6_ functions need netlock. And driver SIOCSIFFLAGS ioctl
> >>>> must not have splnet().
> >>>
> >>> Why not? This is new since the introduction of intr_barrier() or this
> >>> is an old issue?
> >>>
> >>>> Is reducing splnet() the correct aproach?
> >>
> >> yes.
> >>
> >>> I doubt it is possible to answer this question without defining who owns
> >>> `if_flags' and how can it be read/written to.
> >>
> >> NET_LOCK is what "owns" updates to if_flags.
> >
> > Why does reducing splnet() is the correct approach? It isn't clear to
> > me. What's splnet() protecting then?
>
> splnet() and all the other splraise() variants only raise the IPL on the
> current CPU. Unless you have some other lock to coordinate with other CPUs
> (eg KERNEL_LOCK) it doesn't really prevent other code running. ixl in
> particular has mpsafe interrupts, so unless your ioctl code is running on the
> same CPU that ixl is interrupting, it's not helping.
>
> splnet() with KERNEL_LOCK provides backward compat for with legacy drivers.
> The reason it doesn't really help with the network stack is because the stack
> runs from nettq under NET_LOCK without KERNEL_LOCK, it's no longer a softint
> at an IPL lower than net.
The diff discussed in this thread reduces the scope of the splnet/splx()
dance to only surround the modification of `if_flags'. How is this
related to what you said? Is it because `if_flags' is read in interrupt
handlers and it isn't modified atomically? Does that imply that every
modification of `if_flags' should be done at IPL_NET? Does that mean
some love is needed to ensure reading `if_flags' is coherent?
Does that change also imply that it is safe to issue a SIOCSIFFLAGS on
a legacy drivers without blocking interrupts? If the IPL needs to be
raised, this is left to the driver, right?
Was the current splnet/splx() dance an easy way to block packet reception
between multiple SIOCSIFFLAGS ioctls? This might have been a way to not
receive packets on a DOWN interface. This is no longer the case on MP
kernels as there's a window between the UP and DOWN ioctls. Do we care?
Is this down/up/down dance fine for legacy a modern drivers?
> >>>> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> >>>> retrieving revision 1.641
> >>>> diff -u -p -r1.641 if.c
> >>>> --- net/if.c 25 May 2021 22:45:09 -0000 1.641
> >>>> +++ net/if.c 10 Jun 2021 14:32:12 -0000
> >>>> @@ -3109,6 +3109,8 @@ ifnewlladdr(struct ifnet *ifp)
> >>>> short up;
> >>>> int s;
> >>>>
> >>>> + NET_ASSERT_LOCKED();
> >>>> +
> >>>> s = splnet();
> >>>> up = ifp->if_flags & IFF_UP;
> >>>>
> >>>> @@ -3116,11 +3118,14 @@ ifnewlladdr(struct ifnet *ifp)
> >>>> /* go down for a moment... */
> >>>> ifp->if_flags &= ~IFF_UP;
> >>>> ifrq.ifr_flags = ifp->if_flags;
> >>>> + splx(s);
> >>>> (*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
> >>>> + s = splnet();
> >>>> }
> >>>>
> >>>> ifp->if_flags |= IFF_UP;
> >>>> ifrq.ifr_flags = ifp->if_flags;
> >>>> + splx(s);
> >>>> (*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
> >>>>
> >>>> #ifdef INET6
> >>>> @@ -3139,11 +3144,12 @@ ifnewlladdr(struct ifnet *ifp)
> >>>> #endif
> >>>> if (!up) {
> >>>> /* go back down */
> >>>> + s = splnet();
> >>>> ifp->if_flags &= ~IFF_UP;
> >>>> ifrq.ifr_flags = ifp->if_flags;
> >>>> + splx(s);
> >>>> (*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
> >>>> }
> >>>> - splx(s);
> >>>> }
> >>>>
> >>>> void
> >>>>
> >>>
> >>
>
