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.