netdev - Re: [patch net-next 2/3] net: devlink: call lockdep_assert

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: <20220702122946.7bfc387a@kernel.org>
Date:   Sat, 2 Jul 2022 12:29:46 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, pabeni@...hat.com,
        edumazet@...gle.com, mlxsw@...dia.com, saeedm@...dia.com,
        moshe@...dia.com
Subject: Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held()
 for devlink->lock directly

On Sat, 2 Jul 2022 17:58:06 +0200 Jiri Pirko wrote:
> Fri, Jul 01, 2022 at 06:33:16PM CEST, kuba@...nel.org wrote:
> >On Fri,  1 Jul 2022 11:59:25 +0200 Jiri Pirko wrote:  
> >> In devlink.c there is direct access to whole struct devlink so there is
> >> no need to use helper. So obey the customs and work with lock directly
> >> avoiding helpers which might obfuscate things a bit.  
> >  
> >> diff --git a/net/core/devlink.c b/net/core/devlink.c
> >> index 25b481dd1709..a7477addbd59 100644
> >> --- a/net/core/devlink.c
> >> +++ b/net/core/devlink.c
> >> @@ -10185,7 +10185,7 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
> >>  	struct devlink *devlink = devlink_port->devlink;
> >>  	struct devlink_rate *devlink_rate;
> >>  
> >> -	devl_assert_locked(devlink_port->devlink);
> >> +	lockdep_assert_held(&devlink_port->devlink->lock);  
> >
> >I don't understand why. Do we use lockdep asserts directly on rtnl_mutex
> >in rtnetlink.c?  
> 
> Well:
> 
> 1) it's been a long time policy not to use helpers for locks if not
>    needed. There reason is that the reader has easier job in seeing what
>    the code is doing. And here, it is not needed to use helper (we can
>    access the containing struct)

AFAIU the policy is not to _create_ helpers for locks for no good
reason. If the helper already exists it's better to consistently use
it.

> 2) lock/unlock for devlink->lock is done here w/o helpers as well

Existing code, I didn't want to cause major code churn until the
transition is finished.

> 3) there is really no gain of using helper here.

Shorter, easier to type and remember, especially if the author is
already using the exported assert in the driver.

> 4) rtnl_mutex is probably not good example, it has a lot of ancient
>    history behind it.

It's our main lock so we know it best. Do you have other examples?

Look, I don't really care, I just want to make sure we document the
rules of engagement clearly for everyone to see and uniformly enforce. 
So we either need to bash out exactly what we want (and I think our
views differ) or you should switch the commit message to say "I feel
like" rather than referring to "customs" 😁