On 06/07/20(Mon) 15:44, Vitaliy Makkoveev wrote:
> > On 6 Jul 2020, at 12:17, Martin Pieuchot <m...@openbsd.org> wrote:
> > 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?
> 
> We should be sure there is no multiple `ifnet’s in `if_list’ with the same
> `if_xname’.

That's a symptom of a bug.  Checking for a symptom won't prevent another
type of corruption, maybe next time it will be a corrupted pointer?

>             And the assertion you proposed looks not obvious here.

Why, is it because of the if() check?  That's required unless we change
put all if_attach() functions under the lock which would require changing
all driver in-tree.  However since drivers for physical devices are being
attached without having multiple CPUs running there's no possible race.

> Assertion like below looks more reasonable but introduces performance
> impact.

We should first aim for correctness then performance.  In this case,
performance is not even an issue because interfaces are not created
often compared to the rate of processing packets.

Reply via email to