Hi Linus,
please pull a single core/urgent fix for 5.16.
Thx.
---
The following changes since commit cabdc3a8475b918e55744f43719b26a82dc8fa6b:
sched,x86: Don't use cluster topology for x86 hybrid CPUs (2021-12-08 22:15:37 +0100)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/core_urgent_for_v5.16_rc6
for you to fetch changes up to 6c3118c32129b4197999a8928ba776bcabd0f5c4:
signal: Skip the altstack update when not needed (2021-12-14 13:08:36 -0800)
----------------------------------------------------------------
- Prevent lock contention on the new sigaltstack lock on the common-case
path, when no changes have been made to the alternative signal stack.
----------------------------------------------------------------
Chang S. Bae (1):
signal: Skip the altstack update when not needed
kernel/signal.c | 9 +++++++++
1 file changed, 9 insertions(+)
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Ivo Totev, HRB 36809, AG Nürnberg
On Sun, Dec 19, 2021 at 5:40 AM Borislav Petkov <[email protected]> wrote:
>
> Prevent lock contention on the new sigaltstack lock on the common-case
> path, when no changes have been made to the alternative signal stack.
I pulled this, but I think it's wrong.
It checks whether the new ss_size/ss_sp is the same as the old ones,
and skips the work if so.
But that check is bogus.
Why? Because it's comparing the wrong values.
It's comparing the values as they are *before* they are fixed up for
the SS_DISABLE case.
So if the new mode is SS_DISABLE, it's going to compare the values
against the old values for ss_size and ss_sp: but those old values
will have been reset to 0/NULL.
And the new values have not been reset yet before the comparison.
So the comparison wil easily fail when it shouldn't.
And that's all pointless anyway. If it's SS_DISABLE, there's no point
in doing *any* of this.
Now, I decided to keep the pull, because this bug only means that the
commit isn't actually as effective as it *should* be.
Honestly, that do_sigaltstack code is written just about pessimally,
and taking the sigaltstack_lock just for the limit checking is all
kinds of silly.
The SS_DISABLE case shouldn't take the lock at all.
And the actual modification of the values shouldn't need any locking
at all, since it's all thread-local.
I'm not convinced even the limit checking needs the lock, but
whatever. I think it could maybe just use "read_once()" or something.
I think the attached patch is an improvement, but I did *not* test
this, and I'm just throwing this out as a "maybe something like this".
Comments?
Note: I will throw this patch away after sending it out. If people agree,
Linus
On Sun, Dec 19, 2021 at 12:14 PM Linus Torvalds
<[email protected]> wrote:
>
> Note: I will throw this patch away after sending it out. If people agree,
Gah. I hit send too early. That sentence was supposed to continue: "If
people agree, please apply this with my sign-off after testing".
Linus
The pull request you sent on Sun, 19 Dec 2021 14:40:11 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/core_urgent_for_v5.16_rc6
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/c36d891d787d03b36e18aa4ef254eebe6060b39a
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On 12/19/21 12:14 PM, Linus Torvalds wrote:
...
> The SS_DISABLE case shouldn't take the lock at all.
>
> And the actual modification of the values shouldn't need any locking
> at all, since it's all thread-local.
The modification is definitely thread-local, but the implications are
wider after the dynamic xfeature and sigframe support went in. Now,
(x86-only) no thread is allowed to enable dynamic features unless the
entire _process's_ altstacks pass validate_sigaltstack().
> I'm not convinced even the limit checking needs the lock, but
> whatever. I think it could maybe just use "read_once()" or something.
>
> I think the attached patch is an improvement, but I did *not* test
> this, and I'm just throwing this out as a "maybe something like this".
The patch definitely makes the code easier to read. But, it looks like
we need to invert the sigaltstack_size_valid() condition from the patch:
> + if (unlikely(ss_size < min_ss_size) ||
> + unlikely(sigaltstack_size_valid(ss_size))) {
^^^^^
> + sigaltstack_unlock();
> + return -ENOMEM;
> }
That should be !sigaltstack_size_valid(ss_size).
Also, the sigaltstack_lock() lock really is needed over the assignments
like this:
> t->sas_ss_sp = (unsigned long) ss_sp;
> t->sas_ss_size = ss_size;
> t->sas_ss_flags = ss_flags;
to prevent races with validate_sigaltstack(). We desperately need a
comment in there, but we probably shouldn't reference
validate_sigaltstack() itself since it's deeply x86-only. I've got a
shot at a comment in the attached patch.
As for the the:
> if (ss_mode == SS_DISABLE) {
> t->sas_ss_sp = 0;
> t->sas_ss_size = 0;
> t->sas_ss_flags = ss_flags;
> return 0;
> }
hunk, I think it's OK. Shrinking t->sas_ss_size without the lock is
safe-ish because it will never cause validate_sigaltstack() to fail. I
need to think about that bit more, though.
Another blatantly untested patch is attached. I'll try to give it a go
on some real hardware tomorrow.
On Sun, Dec 19, 2021 at 9:25 PM Dave Hansen <[email protected]> wrote:
>
> The patch definitely makes the code easier to read. But, it looks like
> we need to invert the sigaltstack_size_valid() condition from the patch:
Yup, that's just me messign up when moving code around and adding the
second "unlikely()",
> Also, the sigaltstack_lock() lock really is needed over the assignments
> like this:
>
> > t->sas_ss_sp = (unsigned long) ss_sp;
> > t->sas_ss_size = ss_size;
> > t->sas_ss_flags = ss_flags;
> to prevent races with validate_sigaltstack().
Ugh. This code is garbage. Why the hell does it want a lock for
something stupid like this?
That lock should just be removed entirely as pointless. If some thread
has a sigaltstack that is too small, too bad.
We've never done that validation before, why did people think it was a
good idea to add it now?
And when added, why did people think it had to be done so carefully
under a lock?
Sure, go ahead and make it a "be polite, don't let people ask for
xstate features that won't fit an altstack". But at the point where
people noticed it caused lock contention, just give it up, and do the
unlocked version since it has no actual important semantics.
Whatever. I don't care that much, but this all smells like you just
dug your own hole for very questionable causes, and instead of a
"don't do that then" this all is doubling down on a bad idea.
Linus
On Mon, Dec 20, 2021 at 8:20 AM Linus Torvalds
<[email protected]> wrote:
>
> Whatever. I don't care that much, but this all smells like you just
> dug your own hole for very questionable causes, and instead of a
> "don't do that then" this all is doubling down on a bad idea.
It further looks like it's really only the sas_ss_size that is
checked, so if people wan tto have a lock, make it clear that's the
only thing that the lock is about.
So the actual "do I even need to lock" condition should likely just be
if (ss_size < t->sas_ss_size)
.. don't bother locking ..
but as mentioned, I don't really see much of a point in being so
careful even about the growing case.
If somebody is changing xstate features concurrently with another
thread setting up their altstack, they can keep both pieces.
Linus
On Mon, Dec 20 2021 at 08:25, Linus Torvalds wrote:
> On Mon, Dec 20, 2021 at 8:20 AM Linus Torvalds
> <[email protected]> wrote:
>>
>> Whatever. I don't care that much, but this all smells like you just
>> dug your own hole for very questionable causes, and instead of a
>> "don't do that then" this all is doubling down on a bad idea.
>
> It further looks like it's really only the sas_ss_size that is
> checked, so if people wan tto have a lock, make it clear that's the
> only thing that the lock is about.
>
> So the actual "do I even need to lock" condition should likely just be
>
> if (ss_size < t->sas_ss_size)
> .. don't bother locking ..
>
> but as mentioned, I don't really see much of a point in being so
> careful even about the growing case.
>
> If somebody is changing xstate features concurrently with another
> thread setting up their altstack, they can keep both pieces.
In principle I agree, but the whole signal stack business is a nightmare
and the way how a program ends up using some xfeature is hideous at
best.
An application does not necessarily know about it at all because the
usage is hidden in random library code. So there is a chance to run into
concurrency issues for real.
Let me grab your (Dave's) patch and rework the whole thing into
something sensible. I had a patch around which replaced sighand lock
with an explicit lock for that purpose. Let me dig that out and polish
it all up.
Thanks,
tglx