linux-kernel - Re: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers for interruptible locks

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wgRPDGvofj1PU=NemF6iFu308pFZ=w5P+sQyOMGd978fA@mail.gmail.com>
Date: Fri, 2 May 2025 17:27:40 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Dan Williams <dan.j.williams@...el.com>
Cc: peterz@...radead.org, David Lechner <dlechner@...libre.com>, 
	"Fabio M. De Francesco" <fabio.maria.de.francesco@...ux.intel.com>, Ingo Molnar <mingo@...nel.org>, 
	linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org
Subject: Re: [RFC PATCH] cleanup: Introduce "acquire"/"drop" cleanup helpers
 for interruptible locks

"

On Fri, 2 May 2025 at 16:48, Dan Williams <dan.j.williams@...el.com> wrote:
>
> +       struct mutex *lock __drop(mutex_unlock) =
> +               mutex_intr_acquire(&mds->poison.lock);
> +       if (IS_ERR(lock))
> +               return PTR_ERR(lock);

I do think you'd want to have some macro to make this less full of
random boiler plate.

I'd like to point out that when I said that thing you quote:

    "You should aim for a nice

     struct rw_semaphore *struct rw_semaphore *exec_update_lock
         __cleanup(release_exec_update_lock) = get_exec_update_lock(task);"

that comment was fairly early in the whole cleanup.h process, and
"cond_guard()" - which then turned out to not work very well - was
actually a "let's try to clean that up" thing.

And the real issues have been that people who wanted this actually had
truly ugly code that they wanted to mindlessly try to rewrite to use
cleanup. And in the process it actually just got worse.

So I don't think you should take that early off-the-cuff remark of
mine as some kind of solid advice.

I don't know what the best solution is, but it's almost certainly not
this "write out a lot of boiler plate".

Now, the two things that made me suggest that was not that the code
was pretty, but

 (a) having an explicit cleanup syntax that matches the initial assignment

     ie that "__drop(mutex_unlock)" pairs with mutex_intr_acquire()"

and

 (b) having those two things *together* in the same place

where that (a) was to avoid having arbitrary crazy naming that made no
sense ("__free()" of a lock that you didn't allocate, but that you
took? No thank you), and (b) was to avoid having that situation where
you first assign NULL just because you're going to do the locking
elsewhere, and the lock release function is described in a totally
different place from where we acquire it.

So the goal of "cond_guard()" was to have those things together, but
obviously it just then ended up being very messy.

Your patch may end up fixing some of that failure of cond_guard(), but
at the same time I note that your patch is horribly broken. Look at
your change to drivers/cxl/core/mbox.c: you made it use

+       struct mutex *lock __drop(mutex_unlock) =
+               mutex_intr_acquire(&mds->poison.lock);

but then you didn't remove the existing unlocking, so that function still has

        mutex_unlock(&mds->poison.lock);

at the end. So I think the patch shows that it's really easy to just
mess up the conversion, and that this is certainly not a way to "fix"
things.

               Linus