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: <YsKGZZ8ZggAf+jGT@nanopsycho>
Date:   Mon, 4 Jul 2022 08:19:17 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <kuba@...nel.org>
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

Sat, Jul 02, 2022 at 09:29:46PM CEST, kuba@...nel.org wrote:
>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" 😁

Jakub, I don't really care. If you say we should do it differently, I
will do it differently. I just want the use to be consistent. From the
earlier reactions of DaveM on such helpers, I got an impression we don't
want them if possible. If this is no longer true, I'm fine with it. Just
tell me what I should do.

Thanks!