netdev - Re: [patch net-next RFC 0/2] net: devlink: remove devlink big lock

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]

Message-ID: <YrnPqzKexfgNVC10@shredder>
Date:   Mon, 27 Jun 2022 18:41:31 +0300
From:   Ido Schimmel <idosch@...dia.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        petrm@...dia.com, pabeni@...hat.com, edumazet@...gle.com,
        mlxsw@...dia.com, saeedm@...dia.com
Subject: Re: [patch net-next RFC 0/2] net: devlink: remove devlink big lock

On Mon, Jun 27, 2022 at 03:54:59PM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@...dia.com>
> 
> This is an attempt to remove use of devlink_mutex. This is a global lock
> taken for every user command. That causes that long operations performed
> on one devlink instance (like flash update) are blocking other
> operations on different instances.

This patchset is supposed to prevent one devlink instance from blocking
another? Devlink does not enable "parallel_ops", which means that the
generic netlink mutex is serializing all user space operations. AFAICT,
this series does not enable "parallel_ops", so I'm not sure what
difference the removal of the devlink mutex makes.

The devlink mutex (in accordance with the comment above it) serializes
all user space operations and accesses to the devlink devices list. This
resulted in a AA deadlock in the previous submission because we had a
flow where a user space operation (which acquires this mutex) also tries
to register / unregister a nested devlink instance which also tries to
acquire the mutex.

As long as devlink does not implement "parallel_ops", it seems that the
devlink mutex can be reduced to only serializing accesses to the devlink
devices list, thereby eliminating the deadlock.

> 
> The first patch makes sure that the xarray that holds devlink pointers
> is possible to be safely iterated.
> 
> The second patch moves the user command mutex to be per-devlink.
> 
> Jiri Pirko (2):
>   net: devlink: make sure that devlink_try_get() works with valid
>     pointer during xarray iteration
>   net: devlink: replace devlink_mutex by per-devlink lock
> 
>  net/core/devlink.c | 256 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 161 insertions(+), 95 deletions(-)
> 
> -- 
> 2.35.3
>