On 01/07/20(Wed) 00:02, Vitaliy Makkoveev wrote:
> On Tue, Jun 30, 2020 at 03:48:22PM +0300, Vitaliy Makkoveev wrote:
> > On Tue, Jun 30, 2020 at 12:08:03PM +0200, Martin Pieuchot wrote:
> > > On 29/06/20(Mon) 11:59, Vitaliy Makkoveev wrote:
> > > > [...]
> > > > I reworked tool for reproduce. Now I avoided fork()/exec() route and it
> > > > takes couple of minutes to take panic on 4 cores. Also some screenshots
> > > > attached.
> > >
> > > Setting kern.pool_debug=2 makes the race reproducible in seconds.
>
> Unfortunately you will catch splassert() caused by kern/sched_bsd.c:304.
> malloc() will call yield() while we are holding NET_LOCK(). I attached
> screenshot with splassertion to this mail.
With kern.splassert < 3 it is fine.
> > > Could you turn this test into something committable in regress/? We can
> > > link it to the build once a fix is committed.
> > >
> >
> > We have 3 races with cloned interfaces:
> > 1. if_clone_create() vs if_clone_create()
> > 2. if_clone_destroy() vs if_clone_destroy()
> > 3. if_clone_destroy() vs the rest of stack
> >
> > It makes sences to commit unified test to regress/, so I suggest to wait
> > a little.
>
> The another solution.
>
> Diff below introduces per-`ifc' serialization for if_clone_create() and
> if_clone_destroy(). There is no index bitmap anymore.
I like the simplification. More comments below:
> +/*
> + * Lock a clone network interface.
> + */
> +int
> +if_clone_lock(struct if_clone *ifc)
> +{
> + int error;
> +
> + rw_enter_write(&ifc->ifc_lock);
> +
> + while (ifc->ifc_flags & IFC_CREATE_LOCKED) {
> + ifc->ifc_flags |= IFC_CREATE_LOCKWAIT;
> + error = rwsleep_nsec(&ifc->ifc_flags, &ifc->ifc_lock,
> + PWAIT|PCATCH, "ifclk", INFSLP);
> + if(error != 0) {
> + ifc->ifc_flags &= ~IFC_CREATE_LOCKWAIT;
> + rw_exit_write(&ifc->ifc_lock);
> + return error;
> + }
> + }
> + ifc->ifc_flags |= IFC_CREATE_LOCKED;
> + ifc->ifc_flags &= ~IFC_CREATE_LOCKWAIT;
> +
> + rw_exit_write(&ifc->ifc_lock);
> +
> + return 0;
> +}
This is like re-implementing a rwlock but loosing the debugging ability of
WITNESS.
I also don't see any reason for having a per-ifc lock. If, at least one
of the problems, is a double insert in `ifnet' then we should be able to
assert that a lock is held when doing such assertion.
Assertions and documentation are more important than preventing races
because they allow to build awareness and elegant solutions instead of
hacking diffs until stuff work without knowing why.
There are two cases where `ifp' are inserted into `ifnet':
1. by autoconf during boot or hotplug
2. by cloning ioctl
In the second case it is always about pseudo-devices. So the assertion
should be conditional like:
if (ISSET(ifp->if_xflags, IFXF_CLONED))
rw_assert_wrlock(&ifc_lock);
In other words this fixes serializes insertions/removal on the global
list `ifnet', the KERNEL_LOCK() being still required for reading it.
Is there any other data structure which ends up being protected by this
approach and could be documented?
