linux-kernel - Re: sparse warnings related to kref_put_lock() and refcount_dec_and

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] [day] [month] [year] [list]

Message-ID: <CAK-6q+jngywC7UZ6XG=yww16nJQgz0Z=VYKH8Fhg0kbZCkaypA@mail.gmail.com>
Date:   Wed, 29 Jun 2022 10:42:48 -0400
From:   Alexander Aring <aahringo@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
        jacob.e.keller@...el.com,
        Andrew Morton <akpm@...ux-foundation.org>,
        thunder.leizhen@...wei.com,
        Sparse Mailing-list <linux-sparse@...r.kernel.org>,
        cluster-devel <cluster-devel@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: sparse warnings related to kref_put_lock() and refcount_dec_and_lock()

Hi,

On Tue, Jun 28, 2022 at 1:27 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Tue, Jun 28, 2022 at 1:58 AM Luc Van Oostenryck
> <luc.vanoostenryck@...il.com> wrote:
> >
> > I would certainly not recommend this but ...
> > if it's OK to cheat and lie then you can do:
> > +       bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock) __acquires(lock);
>
> Actually, we have "__cond_lock()" in the kernel to actually document
> that something takes a lock only in certain conditions.
>
> It needs to be declared as a macro in the header file, with this as an example:
>
>    #define raw_spin_trylock(lock)  __cond_lock(lock, _raw_spin_trylock(lock))
>

I added a prefix of "raw_" to refcount_dec_and_lock() and similar
functions and replaced the original functions with the __cond_lock()
macro to redirect to their raw_ functions. Similar to how the
raw_spinlock_trylock() naming scheme is doing it. The "raw_"
functionality should never be called by the user then.

> ie that says that "raw_spin_trylock() takes 'lock' when
> _raw_spin_trylock() returned true".
>
> That then makes it possible to write code like
>
>     if (raw_spin_trylock(lock)) {..
>                  raw_spin_unlock(lock));
>     }
>
> and sparse will get the nesting right.
>
> But that does *not* solve the issue of people then writing this as
>
>     locked = raw_spin_trylock(lock);
>     .. do_something ..
>     if (locked)
>                  raw_spin_unlock(lock));
>
> and you end up with the same thing again.
>

Yes it mostly removed all sparse warnings I see. I needed to move at
one call of the refcount_dec_and_lock() function inside the if
condition and the sparse warning was gone. It should not be a problem
to change it in this way.

If there are no other complaints I will send a patch for the raw_
prefix to all those conditional refcount lock functions and the add a
__cond_lock() macro for the original function calls.

Thanks!

- Alex