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.
