Re: NET_MPSAFE and ether_ioctl()

[Jason Thorpe <thorpej%me.com@localhost>]

> On Jan 16, 2020, at 6:28 AM, Michael van Elst <mlelstv%serpens.de@localhost> wrote:
> 
> thorpej%me.com@localhost (Jason Thorpe) writes:
> 
>> The "wm" driver has the following construct in wm_ioctl():
> 
>> #ifdef WM_MPSAFE
>>               s = splnet();
>> #endif
>>               /* It may call wm_start, so unlock here */
>>               error = ether_ioctl(ifp, cmd, data);
>> #ifdef WM_MPSAFE
>>               splx(s);
>> #endif
> 
>> I'm a little confused as to why it's at all correct for a driver to be using splnet when running in NET_MPSAFE mode.
> 
> 
> It always calls splnet().
> 
> With NET_MPSAFE it calls it for ifmedia_ioctl() and ether_ioctl() as these
> functions aren't MPSAFE.

Actually, it looks to me that ether_ioctl() IS MP-safe, and that's backed up by the fact that no path to ether_ioctl() or inside ether_ioctl() directly takes the KERNEL_LOCK, as it would otherwise need to do; splnet() is NOT sufficient, and I guess my point here is that I think it's not necessary :-). (There are some code paths called BY ether_ioctl() that WILL acquire the KERNEL_LOCK if necessary...)

I understand the issue with ifmedia_ioctl(), and I posted a separate message about that -- a small tweak to make transitioning it to MP-safe a bit easier in the future.

So, what exactly about ether_ioctl() is NOT MP-safe (ignoring the calls it makes to ifmedia_ioctl() -- assume I've dealt with that locally...)?  ether_ioctl() itself is called via wm_ioctl(), which is itself called with the if_ioctl_lock held.  This makes the manipulation of ifp->if_flags (and presumably a bunch of other ifnet fields) safe.  ether_addmulti() and ether_delmulti() themselves use the ec_lock in struct ethercom ... the only thing mildly dodgy there is manipulation of ec_capenable ... but one could argue that's protected by the if_ioctl_lock, and so someone who knows the intent should please confirm or refute that (FWIW, it would be nice to document the ethercom locking protocol ... and I will take a stab at doing so once I have my head fully wrapped around it.)

The calls into ifioctl_common() also appear MP-safe, and for the portions that aren't, the KERNEL_LOCK is acquired as needed...

So, again, I'm failing to see how ether_ioctl() itself needs to have calls to it guarded with splnet() in the NET_MPSAFE case.  (I completely understand that it needs to be guarded with splnet() in the non-NET_MPSAFE case.)

> Without NET_MPSAFE the whole wm_ioctl is protected as there are no mutexes.
> 
> The ugliness goes away when someone cares about the ifmedia and ether
> functions.

Again, I've reduced the ugliness around ifmedia_ioctl() locally (and posted the proposed patch) ... my goal here is to make it easier to transition drivers to the NET_MPSAFE world without littering them with gobs of #ifdefs.  (I will be proposing another measure that will allow drivers to get off of (*if_watchdog)() more easily, and make the transition to the new thing basically mechanical.)

-- thorpej