* [GIT PULL] core/urgent for v5.16-rc6 @ 2021-12-19 13:40 Borislav Petkov 2021-12-19 20:14 ` Linus Torvalds 2021-12-19 20:34 ` pr-tracker-bot 0 siblings, 2 replies; 8+ messages in thread From: Borislav Petkov @ 2021-12-19 13:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: x86-ml, lkml 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] core/urgent for v5.16-rc6 2021-12-19 13:40 [GIT PULL] core/urgent for v5.16-rc6 Borislav Petkov @ 2021-12-19 20:14 ` Linus Torvalds 2021-12-19 20:16 ` Linus Torvalds 2021-12-20 5:25 ` Dave Hansen 2021-12-19 20:34 ` pr-tracker-bot 1 sibling, 2 replies; 8+ messages in thread From: Linus Torvalds @ 2021-12-19 20:14 UTC (permalink / raw) To: Borislav Petkov, Chang S. Bae, Dave Hansen, Thomas Gleixner; +Cc: x86-ml, lkml [-- Attachment #1: Type: text/plain, Size: 1719 bytes --] On Sun, Dec 19, 2021 at 5:40 AM Borislav Petkov <bp@suse.de> 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 [-- Attachment #2: patch.diff --] [-- Type: text/x-patch, Size: 1830 bytes --] kernel/signal.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/kernel/signal.c b/kernel/signal.c index dfcee3888b00..f58f1d574931 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -4161,7 +4161,6 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp, size_t min_ss_size) { struct task_struct *t = current; - int ret = 0; if (oss) { memset(oss, 0, sizeof(stack_t)); @@ -4181,8 +4180,15 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp, return -EPERM; ss_mode = ss_flags & ~SS_FLAG_BITS; - if (unlikely(ss_mode != SS_DISABLE && ss_mode != SS_ONSTACK && - ss_mode != 0)) + + if (ss_mode == SS_DISABLE) { + t->sas_ss_sp = 0; + t->sas_ss_size = 0; + t->sas_ss_flags = ss_flags; + return 0; + } + + if (unlikely(ss_mode != SS_ONSTACK && ss_mode != 0)) return -EINVAL; /* @@ -4194,24 +4200,20 @@ do_sigaltstack (const stack_t *ss, stack_t *oss, unsigned long sp, t->sas_ss_flags == ss_flags) return 0; + /* Is this lock really worth it? */ sigaltstack_lock(); - if (ss_mode == SS_DISABLE) { - ss_size = 0; - ss_sp = NULL; - } else { - if (unlikely(ss_size < min_ss_size)) - ret = -ENOMEM; - if (!sigaltstack_size_valid(ss_size)) - ret = -ENOMEM; - } - if (!ret) { - t->sas_ss_sp = (unsigned long) ss_sp; - t->sas_ss_size = ss_size; - t->sas_ss_flags = ss_flags; + if (unlikely(ss_size < min_ss_size) || + unlikely(sigaltstack_size_valid(ss_size))) { + sigaltstack_unlock(); + return -ENOMEM; } sigaltstack_unlock(); + + t->sas_ss_sp = (unsigned long) ss_sp; + t->sas_ss_size = ss_size; + t->sas_ss_flags = ss_flags; } - return ret; + return 0; } SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss) ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [GIT PULL] core/urgent for v5.16-rc6 2021-12-19 20:14 ` Linus Torvalds @ 2021-12-19 20:16 ` Linus Torvalds 2021-12-20 5:25 ` Dave Hansen 1 sibling, 0 replies; 8+ messages in thread From: Linus Torvalds @ 2021-12-19 20:16 UTC (permalink / raw) To: Borislav Petkov, Chang S. Bae, Dave Hansen, Thomas Gleixner; +Cc: x86-ml, lkml On Sun, Dec 19, 2021 at 12:14 PM Linus Torvalds <torvalds@linux-foundation.org> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] core/urgent for v5.16-rc6 2021-12-19 20:14 ` Linus Torvalds 2021-12-19 20:16 ` Linus Torvalds @ 2021-12-20 5:25 ` Dave Hansen 2021-12-20 16:20 ` Linus Torvalds 1 sibling, 1 reply; 8+ messages in thread From: Dave Hansen @ 2021-12-20 5:25 UTC (permalink / raw) To: Linus Torvalds, Borislav Petkov, Chang S. Bae, Dave Hansen, Thomas Gleixner Cc: x86-ml, lkml [-- Attachment #1: Type: text/plain, Size: 2232 bytes --] 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. [-- Attachment #2: patch2.diff --] [-- Type: text/x-patch, Size: 2089 bytes --] 1 file changed, 19 insertions(+), 17 deletions(-) index dfcee3888b00..f58f1d574931 100644 --- b/kernel/signal.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff -puN kernel/signal.c~linux-sigaltstack kernel/signal.c --- a/kernel/signal.c~linux-sigaltstack 2021-12-19 16:50:41.411762535 -0800 +++ b/kernel/signal.c 2021-12-19 21:14:14.605399136 -0800 @@ -4161,7 +4161,6 @@ do_sigaltstack (const stack_t *ss, stack size_t min_ss_size) { struct task_struct *t = current; - int ret = 0; if (oss) { memset(oss, 0, sizeof(stack_t)); @@ -4181,8 +4180,15 @@ do_sigaltstack (const stack_t *ss, stack return -EPERM; ss_mode = ss_flags & ~SS_FLAG_BITS; - if (unlikely(ss_mode != SS_DISABLE && ss_mode != SS_ONSTACK && - ss_mode != 0)) + + if (ss_mode == SS_DISABLE) { + t->sas_ss_sp = 0; + t->sas_ss_size = 0; + t->sas_ss_flags = ss_flags; + return 0; + } + + if (unlikely(ss_mode != SS_ONSTACK && ss_mode != 0)) return -EINVAL; /* @@ -4194,24 +4200,25 @@ do_sigaltstack (const stack_t *ss, stack t->sas_ss_flags == ss_flags) return 0; + /* + * Lock out any changes to sigaltstack_size_valid() + * until the t->sas_ss_* changes are complete: + */ sigaltstack_lock(); - if (ss_mode == SS_DISABLE) { - ss_size = 0; - ss_sp = NULL; - } else { - if (unlikely(ss_size < min_ss_size)) - ret = -ENOMEM; - if (!sigaltstack_size_valid(ss_size)) - ret = -ENOMEM; - } - if (!ret) { - t->sas_ss_sp = (unsigned long) ss_sp; - t->sas_ss_size = ss_size; - t->sas_ss_flags = ss_flags; + + if (unlikely(ss_size < min_ss_size) || + unlikely(!sigaltstack_size_valid(ss_size))) { + sigaltstack_unlock(); + return -ENOMEM; } + + t->sas_ss_sp = (unsigned long) ss_sp; + t->sas_ss_size = ss_size; + t->sas_ss_flags = ss_flags; + sigaltstack_unlock(); } - return ret; + return 0; } SYSCALL_DEFINE2(sigaltstack,const stack_t __user *,uss, stack_t __user *,uoss) _ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] core/urgent for v5.16-rc6 2021-12-20 5:25 ` Dave Hansen @ 2021-12-20 16:20 ` Linus Torvalds 2021-12-20 16:25 ` Linus Torvalds 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2021-12-20 16:20 UTC (permalink / raw) To: Dave Hansen Cc: Borislav Petkov, Chang S. Bae, Dave Hansen, Thomas Gleixner, x86-ml, lkml On Sun, Dec 19, 2021 at 9:25 PM Dave Hansen <dave.hansen@intel.com> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] core/urgent for v5.16-rc6 2021-12-20 16:20 ` Linus Torvalds @ 2021-12-20 16:25 ` Linus Torvalds 2022-01-10 19:01 ` Thomas Gleixner 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2021-12-20 16:25 UTC (permalink / raw) To: Dave Hansen Cc: Borislav Petkov, Chang S. Bae, Dave Hansen, Thomas Gleixner, x86-ml, lkml On Mon, Dec 20, 2021 at 8:20 AM Linus Torvalds <torvalds@linux-foundation.org> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] core/urgent for v5.16-rc6 2021-12-20 16:25 ` Linus Torvalds @ 2022-01-10 19:01 ` Thomas Gleixner 0 siblings, 0 replies; 8+ messages in thread From: Thomas Gleixner @ 2022-01-10 19:01 UTC (permalink / raw) To: Linus Torvalds, Dave Hansen Cc: Borislav Petkov, Chang S. Bae, Dave Hansen, x86-ml, lkml On Mon, Dec 20 2021 at 08:25, Linus Torvalds wrote: > On Mon, Dec 20, 2021 at 8:20 AM Linus Torvalds > <torvalds@linux-foundation.org> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [GIT PULL] core/urgent for v5.16-rc6 2021-12-19 13:40 [GIT PULL] core/urgent for v5.16-rc6 Borislav Petkov 2021-12-19 20:14 ` Linus Torvalds @ 2021-12-19 20:34 ` pr-tracker-bot 1 sibling, 0 replies; 8+ messages in thread From: pr-tracker-bot @ 2021-12-19 20:34 UTC (permalink / raw) To: Borislav Petkov; +Cc: Linus Torvalds, x86-ml, lkml 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-01-10 19:01 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-19 13:40 [GIT PULL] core/urgent for v5.16-rc6 Borislav Petkov 2021-12-19 20:14 ` Linus Torvalds 2021-12-19 20:16 ` Linus Torvalds 2021-12-20 5:25 ` Dave Hansen 2021-12-20 16:20 ` Linus Torvalds 2021-12-20 16:25 ` Linus Torvalds 2022-01-10 19:01 ` Thomas Gleixner 2021-12-19 20:34 ` pr-tracker-bot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).