On Thu, Aug 31, 2023 at 01:42:45PM +0200, Alexander Bluhm wrote:
> On Thu, Aug 31, 2023 at 01:05:11PM +0300, Vitaliy Makkoveev wrote:
> > On Thu, Aug 31, 2023 at 11:26:42AM +0200, Jeremie Courreges-Anglas wrote:
> > >
> > > Looks umb(4) triggers the NET_ASSERT_LOCKED() check in
> > > rtable_getsource() when the umb(4) interface comes up (here with
> > > kern.splassert=2 to get context). Reproduced with GENERIC.MP from Aug
> > > 28 as well with cvs HEAD/if_umb.c rev 1.54.
> > >
> > > Something to worry about?
> > >
> > >
> > > OpenBSD 7.3-current (GENERIC.MP) #1357: Mon Aug 28 20:14:09 MDT 2023
> > > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > > [...]
> > > umb0 at uhub0 port 3 configuration 1 interface 0 "FIBOCOM L831-EAU-00"
> > > rev 2.00/17.29 addr 2
> > > [...]
> > > splassert: rtable_getsource: want 2 have 0
> > > Starting stack trace...
> > > rtable_getsource(0,2) at rtable_getsource+0x58
> > > rtm_send(fffffd83b1a817e0,1,0,0) at rtm_send+0xbc
> > > umb_add_inet_config(ffff8000017c7000,edf0e72e,18,1f0e72e) at
> > > umb_add_inet_config+0x2a8
> > > umb_decode_ip_configuration(ffff8000017c7000,ffff800001ccf230,50) at
> > > umb_decode_ip_configuration+0x147
> > > umb_get_response_task(ffff8000017c7000) at umb_get_response_task+0xda
> > > usb_task_thread(ffff800022fe0010) at usb_task_thread+0xe5
> > > end trace frame: 0x0, count: 251
> > > End of stack trace.
> > >
> >
> > rtable_getsource() requires at least shared netlock to be held. It can't
> > be taken within rtm_send() because we have paths where caller already
> > holds it.
>
> I am not sure if rtm_miss() a few lines above should run without
> netlock. Could we just move the NET_UNLOCK() currenly above the
> if block after the else block?
>
I could miss something, but I don't see rtm_miss() and rtm_msg1() touch
rti_ifa which is netlock protected. The rest of info data is local to
umb_add_inet{,6}_config() or immutable, so nothing requires netlock
here. route_input() is just mbuf allocation and transmission to the
route sockets.
> NET_UNLOCK() and NET_LOCK_SHARED() just after each other does not
> make much sense. Just keep exclusive netlock for the few lines.
>
Agreed. Both the cases perform route sockets walkthrough and message
transmission. No sense for lockless error path only.
Index: sys/dev/usb/if_umb.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
retrieving revision 1.54
diff -u -p -r1.54 if_umb.c
--- sys/dev/usb/if_umb.c 29 Aug 2023 23:28:38 -0000 1.54
+++ sys/dev/usb/if_umb.c 31 Aug 2023 13:20:21 -0000
@@ -1851,7 +1851,6 @@ umb_add_inet_config(struct umb_softc *sc
info.rti_info[RTAX_GATEWAY] = sintosa(&ifra.ifra_dstaddr);
rv = rtrequest(RTM_ADD, &info, 0, &rt, ifp->if_rdomain);
- NET_UNLOCK();
if (rv) {
printf("%s: unable to set IPv4 default route, "
"error %d\n", DEVNAM(ifp->if_softc), rv);
@@ -1862,6 +1861,7 @@ umb_add_inet_config(struct umb_softc *sc
rtm_send(rt, RTM_ADD, rv, ifp->if_rdomain);
rtfree(rt);
}
+ NET_UNLOCK();
if (ifp->if_flags & IFF_DEBUG) {
char str[3][INET_ADDRSTRLEN];
@@ -1932,7 +1932,6 @@ umb_add_inet6_config(struct umb_softc *s
info.rti_info[RTAX_GATEWAY] = sin6tosa(&ifra.ifra_dstaddr);
rv = rtrequest(RTM_ADD, &info, 0, &rt, ifp->if_rdomain);
- NET_UNLOCK();
if (rv) {
printf("%s: unable to set IPv6 default route, "
"error %d\n", DEVNAM(ifp->if_softc), rv);
@@ -1943,6 +1942,7 @@ umb_add_inet6_config(struct umb_softc *s
rtm_send(rt, RTM_ADD, rv, ifp->if_rdomain);
rtfree(rt);
}
+ NET_UNLOCK();
if (ifp->if_flags & IFF_DEBUG) {
char str[3][INET6_ADDRSTRLEN];
