On Mon, Jul 13, 2020 at 09:53:44AM +0200, Martin Pieuchot wrote: > 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?
Absolutely no. You don't break the list do you understand this? > > > 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. Because we should keep `if_list’ be linked by objects with unique `if_xname’. The modificatios of this list is not the problem. Problem is inconsistency caused by not unique `if_xname'. You are not fixing problem, just simptom. > > > 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. > Well, so let's do "KASSERT(ifunit(ifp->if_xname) == NULL);" here.
