Re: [PATCH net-next] devlink: Require devlink lock during device reload

[Jason Gunthorpe]

On Mon, Nov 15, 2021 at 12:20:21PM +0100, Jiri Pirko wrote:
> Sun, Nov 14, 2021 at 07:19:02AM CET, leon@xxxxxxxxxx wrote:
> >On Fri, Nov 12, 2021 at 08:38:56AM +0100, Jiri Pirko wrote:
> >> Thu, Nov 11, 2021 at 01:17:52PM CET, leon@xxxxxxxxxx wrote:
> >> >On Thu, Nov 11, 2021 at 01:05:11PM +0100, Jiri Pirko wrote:
> >> >> Tue, Nov 09, 2021 at 07:24:27PM CET, jgg@xxxxxxxxxx wrote:
> >> >> >On Tue, Nov 09, 2021 at 08:20:42AM -0800, Jakub Kicinski wrote:
> >> >> >> On Tue, 9 Nov 2021 11:33:35 -0400 Jason Gunthorpe wrote:
> >> >> >> > > > I once sketched out fixing this by removing the need to hold the
> >> >> >> > > > per_net_rwsem just for list iteration, which in turn avoids holding it
> >> >> >> > > > over the devlink reload paths. It seemed like a reasonable step toward
> >> >> >> > > > finer grained locking.  
> >> >> >> > > 
> >> >> >> > > Seems to me the locking is just a symptom.  
> >> >> >> > 
> >> >> >> > My fear is this reload during net ns destruction is devlink uAPI now
> >> >> >> > and, yes it may be only a symptom, but the root cause may be unfixable
> >> >> >> > uAPI constraints.
> >> >> >> 
> >> >> >> If I'm reading this right it locks up 100% of the time, what is a uAPI
> >> >> >> for? DoS? ;)
> >> >> >> 
> >> >> >> Hence my questions about the actual use cases.
> >> >> >
> >> >> >Removing namespace support from devlink would solve the crasher. I
> >> >> >certainly didn't feel bold enough to suggest such a thing :)
> >> >> >
> >> >> >If no other devlink driver cares about this it is probably the best
> >> >> >idea.
> >> >> 
> >> >> Devlink namespace support is not generic, not related to any driver.
> >> >
> >> >What do you mean?
> >> >
> >> >devlink_pernet_pre_exit() calls to devlink reload, which means that only
> >> >drivers that support reload care about it. The reload is driver thing.
> >> 
> >> However, Jason was talking about "namespace support removal from
> >> devlink"..
> >
> >The code that sparkles deadlocks is in devlink_pernet_pre_exit() and
> >this will be nice to remove. I just don't know if it is possible to do
> >without ripping whole namespace support from devlink.
> 
> As discussed offline, the non-standard mlx5/IB usage of network
> namespaces requires non standard mlx5/IB workaround. Does not make any
> sense to remove the devlink net namespace support removal.

Sorry, I don't agree that registering a net notifier in an aux device
probe function is non-standard or wrong.

This model must be supported sanely somehow in the netdev area and
cannot be worked around in leaf drivers.

Intel ice will have the same problem, as would broadcom if they ever
get their driver modernized.

Jason