Re: [PATCH] selftests/x86: Update map_shadow_stack syscall nr

[Edgecombe, Rick P]

+Mark, regarding matching map_shadow_stack syscall numbers.

On Fri, 2023-09-01 at 12:33 -0700, Sohil Mehta wrote:
> Hi Rick,
> 
> On 9/1/2023 11:16 AM, Rick Edgecombe wrote:
> > Shadow stack's selftest utilizes the map_shadow_stack syscall. The
> > syscall is new with the feature, but the selftests cannot
> > automatically
> > find the headers for the kernel source tree they are located in.
> > This
> > resulted in the shadow stack test failing to build until the brand
> > new
> > headers were installed.
> > 
> 
> I am wondering why a definition for __NR_map_shadow_stack is missing
> in
> include/uapi/asm-generic/unistd.h?
> 
> Wouldn't this mean that even if someone were to install the headers
> they
> still wouldn't get the syscall number definition. Am I missing
> something?

There is some autogeneration that happens from the .tbl files.

> 
> > To avoid this, a copy of the new uapi defines needed by the test
> > were
> > included in the selftest (see link for discussion). When shadow
> > stack was
> > merged the syscall number was changed, but the copy in the selftest
> > was
> > not updated.
> > 
> > So update the copy of the syscall number define used when the
> > required
> > headers are not installed, to have the final syscall number from
> > the
> > merge.
> > 
> 
> How about adding a fixes tag to make it a tiny bit easier for someone
> who backports the shstk series?
> 
> Fixes: 81f30337ef4f ("selftests/x86: Add shadow stack test")

I wasn't sure if the proper tag was that commit or the merge one. If
the selftest commit is blamed, and in a backport the original commits
are grabbed plus any that blame them, then the syscall numbers would
end up mismatched.

So I think maybe?
Fixes: df57721f9a63 ("Merge tag 'x86_shstk_for_6.6-rc1' of [...]")

But maybe also this whole duplicate defines thing is questionable.

> 
> > Link: https://lore.kernel.org/lkml/Y%2FijdXoTAATt0+Ct@xxxxxxx/
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > ---
> >  tools/testing/selftests/x86/test_shadow_stack.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/x86/test_shadow_stack.c
> > b/tools/testing/selftests/x86/test_shadow_stack.c
> > index 2188968674cb..757e6527f67e 100644
> > --- a/tools/testing/selftests/x86/test_shadow_stack.c
> > +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> > @@ -40,7 +40,7 @@
> >   * without building the headers.
> >   */
> >  #ifndef __NR_map_shadow_stack
> > -#define __NR_map_shadow_stack  452
> > +#define __NR_map_shadow_stack  453
> >  
> >  #define SHADOW_STACK_SET_TOKEN (1ULL << 0)
> >  
> 
> Reviewed-by: Sohil Mehta <sohil.mehta@xxxxxxxxx>

Thanks!

> 
> Apart from this patch, I think we also need something like commit
> 78252deb023c ("arch: Register fchmodat2, usually as syscall 452") to
> reserve the 453 syscall number for the rest of the architectures.
> 
> Should I send one out if you don't have something prepared already?

Originally there were no other shadow stack features, and so it was
maybe going to be an x86-only syscall. I followed in the footsteps of 
the secret mem syscall, but that one seems to have grown some similar
reservation comments since then. It probably makes more sense for that
one though, since it's sort of a generic functionality. An analogous
x86 specific syscall would maybe be modify_ldt, which doesn't really
have reservations.

But now we also have arm that plans to use it. So maybe it is worth
trying to match syscall numbers? I could imagine scenarios where it
could be useful. And I guess there is also the scenario where a generic
type syscall is added, but only implemented on non-shadow stack
architectures. So then when it makes it's way around, it can't match. I
hadn't thought about it before, so just thinking it through...

So sounds reasonable to me. I don't have anything prepped.

> 
> Thanks,
> Sohil
> 
>