On Fri, Jun 18, 2021 at 07:18:53PM +0200, Alexander Bluhm wrote: > On Thu, Jun 17, 2021 at 12:27:42PM +0300, Vitaliy Makkoveev wrote: > > > Is this the correct version of the diff? Not tested yet. > > It survived a full regress run. > > > Hypothetically we could have the driver for the old NIC which relies > > on disabled interrupts on this ioctl() sequence. > > ifioctl() calls driver's ioctl() without splnet(). Network drivers > raise splnet() in their ioctl function. Pseudo drivers don't care > about splnet(). > > So ifnewlladdr() does not need splnet() around the > (*ifp->if_ioctl)(SIOCSIFFLAGS) call. > > I don't see why interrupts should be disabled for the whole sequence. > The ifconfig down/up/down dance seems to be used for IPv6 duplicate > address detection. We change the MAC and configure a new link local > address. Before we can add it, we need the interface up to send > the neighbor disscovery packet. Those steps are independent regarding > interrupts. > > > dlg@ pointed that this > > splx(9) logic doesn't work on MP machines, but if such NIC was never > > used on MP machines this diff could trigger it. > > Grabbing kernel lock and raising spl together is safe on MP machines. > That is how old drivers work. They use splnet() traditionally and > inherit kernel lock from other layers. >
I'm talking about old uniprocessor machine with something like ne(4) card. In such case we can rely on splnet(9) and mutex(9) only. But according the code ifnewlladdr() is the only place where we perform (*ifp->if_ioctl)(SIOCSIFFLAGS) with disabled interrupts. So I guess we don't have any driver which rely on interrupts disabled outside. Otherwise it was already broken :) > > Also we have the mess around `if_flags' protection, because some code > > should be serialized with `if_flags' modification and such code relies > > on netlock. But also we have some pieces of code which modify `if_flags' > > with the kernel lock held. > > The if_flags modifications should be done with kernel lock. There > are many places where it is done without net lock. We could sprinkle > some KERNEL_ASSERT_LOCKED() to see im my observation is correct. > In some cases like this `if_flags' should be consistent with the driver's state. We can't rely on kernel lock while we perform (*ifp->if_ioctl) because driver can provide context switch. That's why netlock protects such code paths and `if_flags'. The kernel lock is taken because ifioctl() path is still locked and we have both lock taken while we access `if_flags'. But this is not true for all places where we touch `if_flags'. Anyway 'ifnet' structure locking requires some attention. > I would give the diff below a try. Perhaps in snaps? > Yes please. splx(9) logic should go away at least from this layer.
