> On 22 Feb 2019, at 05:01, Martin Pieuchot <m...@openbsd.org> wrote:
>
> On 21/02/19(Thu) 07:35, David Gwynne wrote:
>>> On 20 Feb 2019, at 11:21 pm, Martin Pieuchot <m...@openbsd.org> wrote:
>>>
>>> On 20/02/19(Wed) 14:44, David Gwynne wrote:
>>>> Index: sys/net/if.c
>>>> ===================================================================
>>>> RCS file: /cvs/src/sys/net/if.c,v
>>>> retrieving revision 1.571
>>>> diff -u -p -r1.571 if.c
>>>> --- sys/net/if.c 9 Jan 2019 01:14:21 -0000 1.571
>>>> +++ sys/net/if.c 20 Feb 2019 04:35:42 -0000
>>>> @@ -2143,6 +2143,25 @@ ifioctl(struct socket *so, u_long cmd, c
>>>> NET_UNLOCK();
>>>> break;
>>>>
>>>> + case SIOCSETMPWCFG:
>>>> + case SIOCSPWE3CTRLWORD:
>>>> + case SIOCSPWE3FAT:
>>>> + case SIOCSPWE3NEIGHBOR:
>>>> + case SIOCDPWE3NEIGHBOR:
>>>> + if ((error = suser(p)) != 0)
>>>> + break;
>>>> + /* FALLTHROUGH */
>>>> + case SIOCGETMPWCFG:
>>>> + case SIOCGPWE3CTRLWORD:
>>>> + case SIOCGPWE3FAT:
>>>> + case SIOCGPWE3NEIGHBOR:
>>>> + if_ref(ifp);
>>>> + KERNEL_UNLOCK();
>>>> + error = ((*ifp->if_ioctl)(ifp, cmd, data));
>>>> + KERNEL_LOCK();
>>>> + if_put(ifp);
>>>
>>> Why are you referencing the `ifp' and grabbing the KERNEL_LOCK()
>>> (recursively)?
>>
>> ifioctl gets the ifp pointer from ifunit, which doesn't increase the ref
>> count for you. I'm giving up kernel lock around the pwe3 ioctl calls into
>> the driver, not taking them harder. Taking the ifp ref there guarantees the
>> interface will stay alive^Wallocated over those calls.
>
> It feels premature to me, well I'm confused. None of the other ioctl
> handlers do that. The KERNEL_LOCK() is still held in ifioctl() which
> guarantees serialization. If any of the ioctl(2) calls is going to sleep,
> thus releasing the lock, then I'd suggest to grab the NET_RLOCK() here
> instead. It still guarantees serialization of network ioctls w/ regard
> to detach.
>
> Note that I'll be delighted if you want to remove/push down the NET_LOCK()
> from this code path, but can we keep the handlers coherent?
>
> Even if we're using refcounting, don't we want to serialize all network
> ioctl(2)s? If we're not using the NET_LOCK() for this, can this new lock
> guarantee that that `ifp' isn't going away? Or do you have a better
> idea?
The network stack implicitly taking locks is hurting me way more than it's
helping me at the moment, particularly the net lock, so I would like to spend
some time narrowing that down. If the consensus is that it's too much risk for
drivers to keep themselves consistent then I'd argue for a per ifp rwlock that
the ifioctl code could take and release.
Do you want me to do that for the pwe3 ioctls? There's a small number of MPLS
interfaces, so they'd be good for a test run.
ifunit() is notionally like if_get except it doesn't give you a reference. You
have to be holding a lock that prevents the interface being removed from the
list if you're calling ifunit. The code doesn't make it clear whether the lock
you need to be holding is the kernel lock or the net lock, but the kernel lock
is empirically good enough. If you give up that lock while holding the ifp, you
need to account for your reference to it.
