linux-kernel - Re: [PATCH] signal: Allow tasks to cache one sigqueue struct (again).

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: <CAHk-=wiJ9BoQadX7aefY8LEEZ4CJFTEWpAyKV0CzL3yu3Xybdw@mail.gmail.com>
Date:   Tue, 29 Nov 2022 12:06:22 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Ben Segall <bsegall@...gle.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Mel Gorman <mgorman@...e.de>, Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Valentin Schneider <vschneid@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH] signal: Allow tasks to cache one sigqueue struct (again).

On Tue, Nov 29, 2022 at 10:22 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> That very much means "don't try to revive a patch that was already
> reverted because it was broken and didn't make sense". Throw this
> patch AWAY. It's not worth reviving. Start from scratch, with that
> "this needs to be _obviously_ correct" as the starting point.

Ok, so I realize that I said "obviously correct", and then I'm sending
out this ENTIRELY UNTESTED PATCH as an attachment. And I even made it
not build, on purpose.

In other words, the attached patch is anything *but* obviously
correct, but I'm sending it out as a "start from scratch" suggestion
for "maybe something like this".

Maybe this doesn't catch enough cases to really be worth it, but as a
starting point it

 (a) obviously done under the sighand->siglock or when the task is
dead and cannot be dereferenced

 (b) only adds sigqueues that have SIGQUEUE_PREALLOC set, and with an
empty queue

where (a) makes me at least go "the locking is fairly simple" and (b)
makes me go "and we don't have any rlimit counting issues" and it
really just acts as a single front-end cache.

As a result of (b), the freeing part should be just that kmem_cache_free().

And yeah, that doesn't actually build as-is, because 'sigqueue_cachep'
is local to kernel/signal.c, but rather than fix that I left this just
broken because I get the feeling that __exit_signal() should just be
*moved* into kernel/signal.c, but I didn't think about it enough.

In other words, this is *really* meant as not a good patch, but as a
"can we please go in this direction instead".

And yes, maybe some other "free this siginfo" paths should *also* do
that "try to add it to the cache, and if so, you're done" after doing
the accounting. So this is probably all mis-guided, but I ended up
wanting to just see how an alternative approach would look, and this
feels a lot safer to me.

We already have that SIGQUEUE_PREALLOC case, why not use some of the
same logic for the cached entry. Sure, it bypasses the rlimit, but
what else is new?

Maybe it all needs to do accounting, but at least in this form it
feels "safe", in that you have to free one of those unaccounted
entries in order to get another unaccounted entry.

              Linus

View attachment "patch.diff" of type "text/x-patch" (3047 bytes)