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

Date:   Tue, 28 Jun 2022 10:27:32 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Luc Van Oostenryck <luc.vanoostenryck@...il.com>
Cc:     Alexander Aring <aahringo@...hat.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()

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))

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.

Anyway, for things like refcount_dec_and_lock(), I suspect that first
pattern should work, because you really shouldn't have that second
pattern. If you just decremented without any locking, the actions are
completely different from the "oh, got the last decrement and now it's
locked and I need to free things" or whatever.

                Linus