On Thu, Aug 31, 2023 at 02:44:41PM +0200, Jeremie Courreges-Anglas wrote:
> On Thu, Aug 31 2023, Alexander Bluhm <alexander.bl...@gmx.net> 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?
> >
> > NET_UNLOCK() and NET_LOCK_SHARED() just after each other does not
> > make much sense. Just keep exclusive netlock for the few lines.
>
> This diff fixes the splassert messages but I share blumh's concern about
> granularity. Also, a NET_UNLOCK()/NET_LOCK_SHARED() dance may introduce
> a new sleeping point and a race.
>
umb_add_inet{,6}_config() already have sleep points. Another one doesn't
make something worse.
> If such granularity was useful I guess you folks would have come up with
> a safe exclusive->shared netlock mechanism by now?
>
Do you mean rw_enter(RW_DOWNGRADE)? I see nothing good here.
