Re: [GIT PULL] core/urgent for v5.16-rc6

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- 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)