> On 16 Jun 2021, at 00:39, Martin Pieuchot <m...@openbsd.org> 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.
dlg
>
>>>> Index: net/if.c
>>>> ===================================================================
>>>> 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
>>>>
>>>
>>
