Re: [PATCH v2] signal: add procfd_signal() syscall

On Nov 29, 2018, at 11:55 AM, Christian Brauner [off-list ref] wrote:

quoted

On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote:

quoted

On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner [off-list ref] wrote:

quoted

On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski [off-list ref] wrote:

quoted

On Nov 29, 2018, at 4:28 AM, Florian Weimer [off-list ref]

wrote:

quoted

Disclaimer: I'm looking at this patch because Christian requested it.
I'm not a kernel developer.

* Christian Brauner:

quoted

diff --git a/arch/x86/entry/syscalls/syscall_32.tbl

b/arch/x86/entry/syscalls/syscall_32.tbl

quoted

quoted

index 3cf7b533b3d1..3f27ffd8ae87 100644

--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl

@@ -398,3 +398,4 @@

384    i386    arch_prctl        sys_arch_prctl

__ia32_compat_sys_arch_prctl

quoted

quoted

385    i386    io_pgetevents        sys_io_pgetevents

__ia32_compat_sys_io_pgetevents

quoted

quoted

386    i386    rseq            sys_rseq            __ia32_sys_rseq
+387    i386    procfd_signal        sys_procfd_signal

__ia32_compat_sys_procfd_signal

quoted

quoted

diff --git a/arch/x86/entry/syscalls/syscall_64.tbl

b/arch/x86/entry/syscalls/syscall_64.tbl

quoted

quoted

index f0b1709a5ffb..8a30cde82450 100644

--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl

@@ -343,6 +343,7 @@

332    common    statx            __x64_sys_statx
333    common    io_pgetevents        __x64_sys_io_pgetevents
334    common    rseq            __x64_sys_rseq
+335    64    procfd_signal        __x64_sys_procfd_signal

#
# x32-specific system call numbers start at 512 to avoid cache

impact

quoted

quoted

@@ -386,3 +387,4 @@

545    x32    execveat        __x32_compat_sys_execveat/ptregs
546    x32    preadv2            __x32_compat_sys_preadv64v2
547    x32    pwritev2        __x32_compat_sys_pwritev64v2
+548    x32    procfd_signal        __x32_compat_sys_procfd_signal

Is there a reason why these numbers have to be different?

(See the recent discussion with Andy Lutomirski.)

Hah, I missed this part of the patch.  Let’s not add new x32 syscall
numbers.

Also, can we perhaps rework this a bit to get rid of the compat entry
point?  The easier way would be to check in_compat_syscall(). The nicer
way IMO would be to use the 64-bit structure for 32-bit as well.

Do you have a syscall which set precedence/did this before I could look at?
Just if you happen to remember one.
Fwiw, I followed the other signal syscalls.
They all introduce compat syscalls.

Not really.

Let me try to explain.  I have three issues with the approach in your patchset:

1. You're introducing a new syscall, and it behaves differently on
32-bit and 64-bit because the structure you pass in is different.
This is necessary for old syscalls where compatibility matters, but
maybe we can get rid of it for new syscalls.   Could we define a
siginfo64_t that is identical to the 64-bit siginfo_t and just use
that in all cases?

2. Assuming that #1 doesn't work, then we need compat support.  But
you're doing it by having two different entry points.  Instead, you
could have a single entry point that calls in_compat_syscall() to
decide which structure to read.  This would simplify things because
x86 doesn't really support the separate compat entry points, which
leads me to #3.

3. The separate x32 numbers are a huge turd that may have security
holes and certainly have comprehensibility holes.  I will object to
any patch that adds a new one (like yours).  Fixing #1 or #2 makes
this problem go away.

Does that make any sense?  The #2 fix would be something like:

if (in_compat_syscall)
 copy...user32();
else
 copy_from_user();

The #1 fix would add a copy_siginfo_from_user64() or similar.

Thanks very much! That all helped a bunch already! I'll try to go the
copy_siginfo_from_user64() way first and see if I can make this work. If
we do this I would however only want to use it for the new syscall first
and not change all other signal syscalls over to it too. I'd rather keep
this patchset focussed and small and do such conversions caused by the
new approach later. Does that sound reasonable?

Absolutely. I don’t think we can change old syscalls — the ABI is set in stone. But for new syscalls, I think the always-64-bit behavior makes sense.