linux-kernel - Re: Buggy __free(kfree) usage pattern already in tree

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:   Sat, 16 Sep 2023 00:13:32 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        akpm@...ux-foundation.org
Subject: Re: Buggy __free(kfree) usage pattern already in tree

On Fri, Sep 15, 2023 at 02:50:48PM -0700, Linus Torvalds wrote:
> On Fri, 15 Sept 2023 at 14:32, Peter Zijlstra <peterz@...radead.org> wrote:
> >
> >
> > It also got me thinking about named_guard() for the myriad of
> > conditional locks we have.
> >
> >         named_guard(try_mutex, foo_guard)(&foo->lock);
> >         if (foo_guard) {
> >                 // we got the lock, do our thing
> >         }
> 
> Hmm. It looks ugly to me. I really hate the "named_guard" thing. One
> of the reasons I liked the guard/scoped_guard() macros was because how
> it created _anonymous_ guards, and made it completely unnecessary to
> make up a pointless name.

Yeah, the anonymous thing is very nice.

My ulterior motive for the above was perhaps extending it to allow the
lock argument to be NULL. Which would give us a handy conditional
pattern.

	struct rw_semaphore *exec_update_lock = task ?  &task->exec_update_lock : NULL;
	named_guard(rwsem_read_interruptible, exec_lock_guard)(exec_update_lock);
	if (task && !exec_lock_guard)
		return -EINTR;

And yes, that is definitely not pretty, but it does provide a fairly
wide array of options.

> If trylock ends up being a common pattern, I think we should strive to
> make it a lot easier to use.
> 
> Can we make it act like "scoped_guard()", except the lock function is
> fundamentally conditional?
> 
> Call it "cond_guard()", and make the syntax otherwise be the same as
> "scoped_guard()", iow, using a unique ID for the guard name.
> 
> So
> 
>         cond_guard(try_mutex)(&foo->lock) {
>                 .. this is the "we got the lock" region ..
>         }
> 
> would I think be a much better syntax.
> 
> Could we live with that?

For the trypical use-case that is definitely the more appealing syntax.

Something like:

  #define cond_guard(_name, args...) \
	  for (CLASS(_name, scope)(args), *done = NULL; \
	       !done && scope; done = (void *)1)
	     
works for the simple cases, but something like: try_spinlock_irqsave
would be a bit of a challenge because that would end up with one of
those structs that is not a pointer and they don't cast to a boolean :/

I think I can make it work, I'll go have a play, but perhaps not now,
it's past midnight ;-)