netdev - Re: [PATCH net-next 0/6] Allow parallel devlink execution

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Dec 2021 09:54:55 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Ido Schimmel <idosch@...dia.com>, Jiri Pirko <jiri@...dia.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 0/6] Allow parallel devlink execution

On Tue, Dec 07, 2021 at 08:21:14PM -0800, Jakub Kicinski wrote:
> On Tue, 7 Dec 2021 09:29:03 +0200 Leon Romanovsky wrote:
> > On Mon, Dec 06, 2021 at 06:00:27PM -0800, Jakub Kicinski wrote:
> > > On Sun,  5 Dec 2021 10:22:00 +0200 Leon Romanovsky wrote:  
> > > > This is final piece of devlink locking puzzle, where I remove global
> > > > mutex lock (devlink_mutex), so we can run devlink commands in parallel.
> > > > 
> > > > The series starts with addition of port_list_lock, which is needed to
> > > > prevent locking dependency between netdevsim sysfs and devlink. It
> > > > follows by the patch that adds context aware locking primitives. Such
> > > > primitives allow us to make sure that devlink instance is locked and
> > > > stays locked even during reload operation. The last patches opens
> > > > devlink to parallel commands.  
> > > 
> > > I'm not okay with assuming that all sub-objects are added when devlink
> > > is not registered.  
> > 
> > But none of the patches in this series assume that.
> > 
> > In devlink_nested_lock() patch [1], I added new marker just to make sure
> > that we don't lock if this specific command is called in locked context.
> > 
> > +#define DEVLINK_NESTED_LOCK XA_MARK_2
> > 
> > [1] https://lore.kernel.org/all/2b64a2a81995b56fec0231751ff6075020058584.1638690564.git.leonro@nvidia.com/
> 
> You skip locking if the marker is set. So a register operation can race
> with a user space operation, right?

Not in upstream code.

In upstream code, we call to devlink_*_register()/devlink_*_unregister()
routines in two possible flows: before/after registration or as a part
of user space request through netlink interface. We don't call to them
randomly.

The current code is intermediate solution that allows us to get rid from
devlink_mutex lock together with annotations that help to spot problematic
flows.

In next patches, I will:
1. Reduce scope of devlink->lock to make sure that it locks exactly what
is needed to be protected (linked lists) instead of all-in-one lock as
it is now.
2. Rename devlink->lock to be evlink->lists_lock to clear the mud around
the scope.
3. Untangle mess with pre_doit, where some commands set _FLAG_NEED_*
flags and ignore user_ptr[1]. Every command should take internally the
object they need without any flags. It will make sub-object management
more clear.
4. Push down the mutex_lock(&devlink->lock) pre_doit to actual commands,
so pre_doit won't take any locks at all.
5. Reference count objects or use write semaphore in uregister paths to
make sure that we can access sub-objects without locks. I'm not sure
about the final implementations details yet.

In the steps 3, 4 and 5, we will delete _nested_lock, pre/post doit mess
and make sure that commands are holding as less as possible locks.

I afraid that many here are underestimate the amount of work needed that is
needed in devlink area to clean the rust due-to mixing in-kernel with
user-visible APIs. 

Thanks