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.
> 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.
I would give the diff below a try. Perhaps in snaps?
bluhm
> > Index: net/if.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> > retrieving revision 1.641
> > diff -u -p -r1.641 if.c
> > --- net/if.c 25 May 2021 22:45:09 -0000 1.641
> > +++ net/if.c 16 Jun 2021 23:46:42 -0000
> > @@ -3107,9 +3107,10 @@ ifnewlladdr(struct ifnet *ifp)
> > #endif
> > struct ifreq ifrq;
> > short up;
> > - int s;
> >
> > - s = splnet();
> > + NET_ASSERT_LOCKED(); /* for ioctl and in6 */
> > + KERNEL_ASSERT_LOCKED(); /* for if_flags */
> > +
> > up = ifp->if_flags & IFF_UP;
> >
> > if (up) {
> > @@ -3143,7 +3144,6 @@ ifnewlladdr(struct ifnet *ifp)
> > ifrq.ifr_flags = ifp->if_flags;
> > (*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
> > }
> > - splx(s);
> > }
> >
> > void
> >
