I propose to avoid netlock relocking for SIOCGIFSFFPAGE too.
Also, since the relock should be avoided for many commands, I prefer
to have something like below:
switch(cmd){
case SIOCGIFMEDIA:
case ...:
relock = 1;
break;
}
if (relock)
NET_UNLOCK();
> On 14 Aug 2022, at 22:09, Jeremie Courreges-Anglas <[email protected]> wrote:
>
> On Sun, Aug 14 2022, Jeremie Courreges-Anglas <[email protected]> wrote:
>> On Thu, Jul 28 2022, Alexander Bluhm <[email protected]> wrote:
>>> Hi,
>>>
>>> The netlock for SIOCSIFMEDIA and SIOCGIFMEDIA ioctl is not necessary.
>>> Legacy drivers run with kernel lock, interface media is MP safe or
>>> has kernel lock.
>>>
>>> ixl(4) talks about net lock but in fact has kernel lock.
>>>
>>> Problem is that smtpd(8) periodically checks media status. This
>>> stops network if netlock was taken.
>>>
>>> ok?
>>
>> I'm seeing fallout with cad(4) in the RAMDISK and GENERIC.MP kernels
>> (didn't try GENERIC). RAMDISK after
>>
>> OpenBSD 7.2-beta (RAMDISK) #140: Sat Aug 6 02:06:57 MDT 2022
>>
>> hangs some time after mounting root. On GENERIC.MP I get a panic:
>>
>> --8<--
>> Automatic boot in progress: starting file system checks.
>> /dev/sd0a (c92bac01c037a788.a): file system is clean; not checking
>> /dev/sd0m (c92bac01c037a788.m): file system is clean; not checking
>> /dev/sd0j (c92bac01c037a788.j): file system is clean; not checking
>> /dev/sd0d (c92bac01c037a788.d): file system is clean; not checking
>> /dev/sd0e (c92bac01c037a788.e): file system is clean; not checking
>> /dev/sd0f (c92bac01c037a788.f): file system is clean; not checking
>> /dev/sd0g (c92bac01c037a788.g): file system is clean; not checking
>> /dev/sd0h (c92bac01c037a788.h): file system is clean; not checking
>> /dev/sd0k (c92bac01c037a788.k): file system is clean; not checking
>> /dev/sd0l (c92bac01c037a788.l): file system is clean; not checking
>> /dev/sd0o (c92bac01c037a788.o): file system is clean; not checking
>> /dev/sd0n (c92bac01c037a788.n): file system is clean; not checking
>> /dev/sd0p (c92bac01c037a788.p): file system is clean; not checking
>> pf enabled
>> kern.bufcachepercent: 20 -> 80
>> sysctl: kern.allowdt: value is not available
>> ddb.console: 0 -> 1
>> starting network
>> panic: netlock: lock not held
>> Stopped at panic+0x106: addi a0,zero,256 TID PID UID
>> PR
>> FLAGS PFLAGS CPU COMMAND
>> *464996 1081 0 0x3 0 0 ifconfig
>> 492269 29052 0 0x14000 0x200 1 zerothread
>> panic() at panic+0x106
>> rw_exit_write() at rw_exit_write+0x94
>> cad_ioctl() at cad_ioctl+0x34
>> ifioctl() at ifioctl+0xaaa
>> soo_ioctl() at soo_ioctl+0x152
>> sys_ioctl() at sys_ioctl+0x230
>> svc_handler() at svc_handler+0x284
>> do_trap_user() at do_trap_user+0x15c
>> cpu_exception_handler_user() at cpu_exception_handler_user+0x7c
>> end of kernel
>> end trace frame: 0x5408b2ae8, count: 6
>> https://www.openbsd.org/ddb.html describes the minimum info required in bug
>> reports. Insufficient info makes it difficult to find and fix bugs.
>>
>> -->8--
>>
>> The diff below fixes GENERIC.MP, I have yet to build and try RAMDISK.
>
> Done, the diff fixes bsd.rd too.
>
>> I'm not completely sure this is the best way to solve it but I can't
>> spot other uses of ifp->if_ioctl() not protected by the NET_LOCK() in
>> ifioctl(). Except SIOCGIFSFFPAGE but that seems fine...
>
> Thoughts? ok?
>
>> Index: dev/fdt/if_cad.c
>> ===================================================================
>> RCS file: /home/cvs/src/sys/dev/fdt/if_cad.c,v
>> retrieving revision 1.11
>> diff -u -p -r1.11 if_cad.c
>> --- dev/fdt/if_cad.c 8 Mar 2022 16:13:08 -0000 1.11
>> +++ dev/fdt/if_cad.c 14 Aug 2022 13:04:41 -0000
>> @@ -553,9 +553,11 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
>> int error = 0;
>> int s;
>>
>> - NET_UNLOCK();
>> + if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
>> + NET_UNLOCK();
>> rw_enter_write(&sc->sc_cfg_lock);
>> - NET_LOCK();
>> + if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
>> + NET_LOCK();
>> s = splnet();
>>
>> switch (cmd) {
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
