# Re: \[PATCH v2\] signal: add procfd\_signal() syscall - List: linux-api - 13 messages, 6 authors - Page: https://ratatoskr.run/linux-api/2018/11/145149/t ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Florian Weimer \ - Date: 2018-11-29 23:33:45 - Message: https://ratatoskr.run/linux-api/2018/11/145149.md ```text Disclaimer: I'm looking at this patch because Christian requested it. I'm not a kernel developer. * Christian Brauner: > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl > 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 > 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents > 386 i386 rseq sys_rseq __ia32_sys_rseq > +387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl > 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 > @@ -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.) > +static int do_procfd_signal(int fd, int sig, kernel_siginfo_t *kinfo, int flags, > + bool had_siginfo) > +{ > + int ret; > + struct fd f; > + struct pid *pid; > + > + /* Enforce flags be set to 0 until we add an extension. */ > + if (flags) > + return -EINVAL; > + > + f = fdget_raw(fd); > + if (!f.file) > + return -EBADF; > + > + /* Is this a process file descriptor? */ > + ret = -EINVAL; > + if (!proc_is_tgid_procfd(f.file)) > + goto err; […] > + ret = kill_pid_info(sig, kinfo, pid); I would like to see some comment here what happens to zombie processes. > +/** > + * sys_procfd_signal - send a signal to a process through a process file > + * descriptor > + * @fd: the file descriptor of the process > + * @sig: signal to be sent > + * @info: the signal info > + * @flags: future flags to be passed > + */ > +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user *, info, > + int, flags) Sorry, I'm quite unhappy with the name. “signal” is for signal handler management. procfd_sendsignal, procfd_sigqueueinfo or something like that would be fine. Even procfd_kill would be better IMHO. Looking at the rt_tgsigqueueinfo interface, is there a way to implement the “tg” part with the current procfd_signal interface? Would you use openat to retrieve the Tgid: line from "status"? Thanks, Florian ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Christian Brauner \ - Date: 2018-11-29 19:16:52 - Message: https://ratatoskr.run/linux-api/2018/11/145147.md ```text On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski [ref] wrote: > > >> On Nov 29, 2018, at 4:28 AM, Florian Weimer [ref] >wrote: >> >> Disclaimer: I'm looking at this patch because Christian requested it. >> I'm not a kernel developer. >> >> * Christian Brauner: >> >>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl >b/arch/x86/entry/syscalls/syscall_32.tbl >>> 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 >>> 385 i386 io_pgetevents sys_io_pgetevents >__ia32_compat_sys_io_pgetevents >>> 386 i386 rseq sys_rseq __ia32_sys_rseq >>> +387 i386 procfd_signal sys_procfd_signal >__ia32_compat_sys_procfd_signal >>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl >b/arch/x86/entry/syscalls/syscall_64.tbl >>> 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 >>> @@ -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. ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Andy Lutomirski \ - Date: 2018-11-29 20:14:05 - Message: https://ratatoskr.run/linux-api/2018/11/145144.md ```text > On Nov 29, 2018, at 11:55 AM, Christian Brauner [ref] wrote: > >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote: >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner [ref] wrote: >>> >>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski [ref] wrote: >>>> >>>> >>>>> On Nov 29, 2018, at 4:28 AM, Florian Weimer [ref] >>>> wrote: >>>>> >>>>> Disclaimer: I'm looking at this patch because Christian requested it. >>>>> I'm not a kernel developer. >>>>> >>>>> * Christian Brauner: >>>>> >>>>>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl >>>> b/arch/x86/entry/syscalls/syscall_32.tbl >>>>>> 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 >>>>>> 385 i386 io_pgetevents sys_io_pgetevents >>>> __ia32_compat_sys_io_pgetevents >>>>>> 386 i386 rseq sys_rseq __ia32_sys_rseq >>>>>> +387 i386 procfd_signal sys_procfd_signal >>>> __ia32_compat_sys_procfd_signal >>>>>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl >>>> b/arch/x86/entry/syscalls/syscall_64.tbl >>>>>> 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 >>>>>> @@ -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. ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Andy Lutomirski \ - Date: 2018-11-30 04:00:44 - Message: https://ratatoskr.run/linux-api/2018/11/145148.md ```text > On Nov 29, 2018, at 4:28 AM, Florian Weimer [ref] wrote: > > Disclaimer: I'm looking at this patch because Christian requested it. > I'm not a kernel developer. > > * Christian Brauner: > >> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl >> 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 >> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents >> 386 i386 rseq sys_rseq __ia32_sys_rseq >> +387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal >> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl >> 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 >> @@ -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. ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Andy Lutomirski \ - Date: 2018-11-30 06:29:41 - Message: https://ratatoskr.run/linux-api/2018/11/145146.md ```text On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner [ref] wrote: > > On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski [ref] wrote: > > > > > >> On Nov 29, 2018, at 4:28 AM, Florian Weimer [ref] > >wrote: > >> > >> Disclaimer: I'm looking at this patch because Christian requested it. > >> I'm not a kernel developer. > >> > >> * Christian Brauner: > >> > >>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl > >b/arch/x86/entry/syscalls/syscall_32.tbl > >>> 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 > >>> 385 i386 io_pgetevents sys_io_pgetevents > >__ia32_compat_sys_io_pgetevents > >>> 386 i386 rseq sys_rseq __ia32_sys_rseq > >>> +387 i386 procfd_signal sys_procfd_signal > >__ia32_compat_sys_procfd_signal > >>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl > >b/arch/x86/entry/syscalls/syscall_64.tbl > >>> 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 > >>> @@ -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. ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Christian Brauner \ - Date: 2018-11-30 07:02:40 - Message: https://ratatoskr.run/linux-api/2018/11/145145.md ```text On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote: > On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner [ref] wrote: > > > > On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski [ref] wrote: > > > > > > > > >> On Nov 29, 2018, at 4:28 AM, Florian Weimer [ref] > > >wrote: > > >> > > >> Disclaimer: I'm looking at this patch because Christian requested it. > > >> I'm not a kernel developer. > > >> > > >> * Christian Brauner: > > >> > > >>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl > > >b/arch/x86/entry/syscalls/syscall_32.tbl > > >>> 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 > > >>> 385 i386 io_pgetevents sys_io_pgetevents > > >__ia32_compat_sys_io_pgetevents > > >>> 386 i386 rseq sys_rseq __ia32_sys_rseq > > >>> +387 i386 procfd_signal sys_procfd_signal > > >__ia32_compat_sys_procfd_signal > > >>> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl > > >b/arch/x86/entry/syscalls/syscall_64.tbl > > >>> 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 > > >>> @@ -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? ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Arnd Bergmann \ - Date: 2018-11-30 08:09:17 - Message: https://ratatoskr.run/linux-api/2018/11/145143.md ```text On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski [ref] wrote: > > On Nov 29, 2018, at 11:55 AM, Christian Brauner [ref] wrote: > >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote: > >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner [ref] wrote: > >>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski [ref] wrote: > >> > >> 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. It looks like we already have a 'struct signalfd_siginfo' that is defined in a sane architecture-independent way, so I'd suggest we use that. We may then also want to make sure that any system call that takes a siginfo has a replacement that takes a signalfd_siginfo, and that this replacement can be used to implement the old version purely in user space. Is the current procfd_signal() proposal (under whichever name) sufficient to correctly implement both sys_rt_sigqueueinfo() and sys_rt_tgsigqueueinfo()? Can we implement sys_rt_sigtimedwait() based on signalfd()? If yes, that would leave waitid(), which already needs a replacement for y2038, and that should then also return a signalfd_siginfo. My current preference for waitid() would be to do a version that closely resembles the current interface, but takes a signalfd_siginfo and a __kernel_timespec based rusage replacement (possibly two of them to let us map wait6), but does not operate on procfd or take a signal mask. That would require yet another syscall, but I don't think I can do that before we want to have the set of y2038 safe syscalls. Arnd ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Christian Brauner \ - Date: 2018-11-30 08:42:00 - Message: https://ratatoskr.run/linux-api/2018/11/145142.md ```text On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote: > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski [ref] wrote: > > > On Nov 29, 2018, at 11:55 AM, Christian Brauner [ref] wrote: > > >> On Thu, Nov 29, 2018 at 11:22:58AM -0800, Andy Lutomirski wrote: > > >>> On Thu, Nov 29, 2018 at 11:17 AM Christian Brauner [ref] wrote: > > >>>> On November 30, 2018 5:54:18 AM GMT+13:00, Andy Lutomirski [ref] wrote: > > >> > > >> 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. > > It looks like we already have a 'struct signalfd_siginfo' that is defined in a > sane architecture-independent way, so I'd suggest we use that. Just so that I understand you correctly: swapping out struct signinfo for struct signalfd_siginfo in procfd_? If so that sounds great to me! > > We may then also want to make sure that any system call that takes a > siginfo has a replacement that takes a signalfd_siginfo, and that this > replacement can be used to implement the old version purely in > user space. Sounds good but is unrelated to this patchset I take it. :) > > Is the current procfd_signal() proposal (under whichever name) sufficient > to correctly implement both sys_rt_sigqueueinfo() and sys_rt_tgsigqueueinfo()? Yes, I see no reason why not. My idea is to extend it - after we have a basic version in - to also work with: /proc//task/ If I'm not mistaken this should be sufficient to get rt_tgsigqueueinfo. The thread will be uniquely identified by the tid descriptor and no combination of /proc/ and /proc//task/ is needed. Does that sound reasonable? > Can we implement sys_rt_sigtimedwait() based on signalfd()? > If yes, that would leave waitid(), which already needs a replacement > for y2038, and that should then also return a signalfd_siginfo. > My current preference for waitid() would be to do a version that > closely resembles the current interface, but takes a signalfd_siginfo > and a __kernel_timespec based rusage replacement (possibly > two of them to let us map wait6), but does not operate on procfd or > take a signal mask. That would require yet another syscall, but I > don't think I can do that before we want to have the set of y2038 > safe syscalls. All sounds reasonable to me but that's not a blocker for the current syscall though, is it? Christian ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Arnd Bergmann \ - Date: 2018-11-30 08:47:58 - Message: https://ratatoskr.run/linux-api/2018/11/145141.md ```text On Thu, Nov 29, 2018 at 10:35 PM Christian Brauner [ref] wrote: > On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote: > > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski [ref] wrote: > > > > Is the current procfd_signal() proposal (under whichever name) sufficient > > to correctly implement both sys_rt_sigqueueinfo() and sys_rt_tgsigqueueinfo()? > > Yes, I see no reason why not. My idea is to extend it - after we have a > basic version in - to also work with: > /proc//task/ > If I'm not mistaken this should be sufficient to get rt_tgsigqueueinfo. > The thread will be uniquely identified by the tid descriptor and no > combination of /proc/ and /proc//task/ is needed. Does > that sound reasonable? Yes. So it would currently replace rt_gsigqueueinfo() but not rt_tgsigqueueinfo(), and could be extended to do both afterwards, without making the interface ugly in any form? I suppose we can always add more flags if needed, and you already ensure that flags is zero for the moment. > > Can we implement sys_rt_sigtimedwait() based on signalfd()? > > > > If yes, that would leave waitid(), which already needs a replacement > > for y2038, and that should then also return a signalfd_siginfo. > > My current preference for waitid() would be to do a version that > > closely resembles the current interface, but takes a signalfd_siginfo > > and a __kernel_timespec based rusage replacement (possibly > > two of them to let us map wait6), but does not operate on procfd or > > take a signal mask. That would require yet another syscall, but I > > don't think I can do that before we want to have the set of y2038 > > safe syscalls. > > All sounds reasonable to me but that's not a blocker for the current > syscall though, is it? I'd like to at least understand about sys_rt_sigtimedwait() before we go on, so we all know what's coming, and document the plans in the changelog. waitid() probably remains on my plate anyway, and I hope understand where we're at with it. Arnd ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Aleksa Sarai \ - Date: 2018-11-30 13:48:11 - Message: https://ratatoskr.run/linux-api/2018/11/145140.md ```text On 2018-11-29, Arnd Bergmann [ref] wrote: > waitid() probably remains on my plate anyway, and I hope understand > where we're at with it. Having a way to wait on a processfd is something we'll eventually need, though the semantics of zombies might get a little bit hairy. I propose we work through that rewrite in a future series once this one goes in. -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Christian Brauner \ - Date: 2018-12-01 11:03:40 - Message: https://ratatoskr.run/linux-api/2018/12/146841.md ```text On November 30, 2018 1:28:15 AM GMT+13:00, Florian Weimer [ref] wrote: >Disclaimer: I'm looking at this patch because Christian requested it. >I'm not a kernel developer. Given all your expertise this really doesn't matter. :) You're the one having to deal with this in glibc after all. Thanks for doing this and sorry for the late reply. I missed that mail. > >* Christian Brauner: > >> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl >b/arch/x86/entry/syscalls/syscall_32.tbl >> 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 >> >385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents >> 386 i386 rseq sys_rseq __ia32_sys_rseq >> >+387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal >> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl >b/arch/x86/entry/syscalls/syscall_64.tbl >> 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 >> @@ -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.) > >> +static int do_procfd_signal(int fd, int sig, kernel_siginfo_t >*kinfo, int flags, >> + bool had_siginfo) >> +{ >> + int ret; >> + struct fd f; >> + struct pid *pid; >> + >> + /* Enforce flags be set to 0 until we add an extension. */ >> + if (flags) >> + return -EINVAL; >> + >> + f = fdget_raw(fd); >> + if (!f.file) >> + return -EBADF; >> + >> + /* Is this a process file descriptor? */ >> + ret = -EINVAL; >> + if (!proc_is_tgid_procfd(f.file)) >> + goto err; >[…] >> + ret = kill_pid_info(sig, kinfo, pid); > >I would like to see some comment here what happens to zombie processes. You'd get ESRCH. I'm not sure if that has always been the case. Eric recently did some excellent refactoring of the signal code. Iirc, part of that involved not delivering signals to zombies. That's at least how I remember it. I don't have access to source code though atm. > >> +/** >> + * sys_procfd_signal - send a signal to a process through a process >file >> + * descriptor >> + * @fd: the file descriptor of the process >> + * @sig: signal to be sent >> + * @info: the signal info >> + * @flags: future flags to be passed >> + */ >> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user >*, info, >> + int, flags) > >Sorry, I'm quite unhappy with the name. “signal” is for signal handler >management. procfd_sendsignal, procfd_sigqueueinfo or something like >that would be fine. Even procfd_kill would be better IMHO. Ok. I only have strong opinions to procfd_kill(). Mainly because the new syscall takes the job of multiple other syscalls so kill gives the wrong impression. I'll come up with a better name in the next iteration. > >Looking at the rt_tgsigqueueinfo interface, is there a way to implement >the “tg” part with the current procfd_signal interface? Would you use >openat to retrieve the Tgid: line from "status"? Yes, the tg part can be implemented. As I pointed out in another mail my I is to make this work by using file descriptors for /proc//task/. I don't want this in the initial patchset though. I prefer to slowly add those features once we have gotten the basic functionality in. > >Thanks, >Florian ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Christian Brauner \ - Date: 2018-12-01 12:36:49 - Message: https://ratatoskr.run/linux-api/2018/12/146838.md ```text On November 30, 2018 10:40:49 AM GMT+13:00, Arnd Bergmann [ref] wrote: >On Thu, Nov 29, 2018 at 10:35 PM Christian Brauner >[ref] wrote: >> On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote: >> > On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski >[ref] wrote: >> > >> > Is the current procfd_signal() proposal (under whichever name) >sufficient >> > to correctly implement both sys_rt_sigqueueinfo() and >sys_rt_tgsigqueueinfo()? >> >> Yes, I see no reason why not. My idea is to extend it - after we have >a >> basic version in - to also work with: >> /proc//task/ >> If I'm not mistaken this should be sufficient to get >rt_tgsigqueueinfo. >> The thread will be uniquely identified by the tid descriptor and no >> combination of /proc/ and /proc//task/ is needed. Does >> that sound reasonable? > >Yes. So it would currently replace rt_gsigqueueinfo() but >not rt_tgsigqueueinfo(), and could be extended to do both >afterwards, without making the interface ugly in any form? Yes. :) > >I suppose we can always add more flags if needed, and you >already ensure that flags is zero for the moment. Yep. > >> > Can we implement sys_rt_sigtimedwait() based on signalfd()? >> > >> > If yes, that would leave waitid(), which already needs a >replacement >> > for y2038, and that should then also return a signalfd_siginfo. >> > My current preference for waitid() would be to do a version that >> > closely resembles the current interface, but takes a >signalfd_siginfo >> > and a __kernel_timespec based rusage replacement (possibly >> > two of them to let us map wait6), but does not operate on procfd or >> > take a signal mask. That would require yet another syscall, but I >> > don't think I can do that before we want to have the set of y2038 >> > safe syscalls. >> >> All sounds reasonable to me but that's not a blocker for the current >> syscall though, is it? > >I'd like to at least understand about sys_rt_sigtimedwait() before >we go on, so we all know what's coming, and document the >plans in the changelog. > >waitid() probably remains on my plate anyway, and I hope understand >where we're at with it. > > Arnd ``` ## Re: \[PATCH v2\] signal: add procfd\_signal() syscall - From: Christian Brauner \ - Date: 2018-12-02 10:03:17 - Message: https://ratatoskr.run/linux-api/2018/12/147178.md ```text On Sat, Dec 01, 2018 at 12:52:24PM +1300, Christian Brauner wrote: > On November 30, 2018 1:28:15 AM GMT+13:00, Florian Weimer [ref] wrote: > >Disclaimer: I'm looking at this patch because Christian requested it. > >I'm not a kernel developer. > > Given all your expertise this really doesn't matter. :) > You're the one having to deal with this > in glibc after all. > Thanks for doing this and sorry for the late reply. > I missed that mail. > > > > >* Christian Brauner: > > > >> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl > >b/arch/x86/entry/syscalls/syscall_32.tbl > >> 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 > >> > >385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents > >> 386 i386 rseq sys_rseq __ia32_sys_rseq > >> > >+387 i386 procfd_signal sys_procfd_signal __ia32_compat_sys_procfd_signal > >> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl > >b/arch/x86/entry/syscalls/syscall_64.tbl > >> 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 > >> @@ -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.) > > > >> +static int do_procfd_signal(int fd, int sig, kernel_siginfo_t > >*kinfo, int flags, > >> + bool had_siginfo) > >> +{ > >> + int ret; > >> + struct fd f; > >> + struct pid *pid; > >> + > >> + /* Enforce flags be set to 0 until we add an extension. */ > >> + if (flags) > >> + return -EINVAL; > >> + > >> + f = fdget_raw(fd); > >> + if (!f.file) > >> + return -EBADF; > >> + > >> + /* Is this a process file descriptor? */ > >> + ret = -EINVAL; > >> + if (!proc_is_tgid_procfd(f.file)) > >> + goto err; > >[…] > >> + ret = kill_pid_info(sig, kinfo, pid); > > > >I would like to see some comment here what happens to zombie processes. > > You'd get ESRCH. > I'm not sure if that has always been the case. > Eric recently did some excellent refactoring of the signal code. > Iirc, part of that involved not delivering signals to zombies. > That's at least how I remember it. > I don't have access to source code though atm. Ok, I finally have access to source code again. Scratch what I said above! I looked at the code and tested it. If the process has exited but not yet waited upon aka is a zombie procfd_send_signal() will return 0. This is identical to kill(2) behavior. It should've been sort-of obvious since when a process is in zombie state /proc/ will still be around which means that struct pid must still be around. > > > > >> +/** > >> + * sys_procfd_signal - send a signal to a process through a process > >file > >> + * descriptor > >> + * @fd: the file descriptor of the process > >> + * @sig: signal to be sent > >> + * @info: the signal info > >> + * @flags: future flags to be passed > >> + */ > >> +SYSCALL_DEFINE4(procfd_signal, int, fd, int, sig, siginfo_t __user > >*, info, > >> + int, flags) > > > >Sorry, I'm quite unhappy with the name. “signal” is for signal handler > >management. procfd_sendsignal, procfd_sigqueueinfo or something like > >that would be fine. Even procfd_kill would be better IMHO. > > Ok. I only have strong opinions to procfd_kill(). > Mainly because the new syscall takes > the job of multiple other syscalls > so kill gives the wrong impression. > I'll come up with a better name in the next iteration. > > > > >Looking at the rt_tgsigqueueinfo interface, is there a way to implement > >the “tg” part with the current procfd_signal interface? Would you use > >openat to retrieve the Tgid: line from "status"? > > Yes, the tg part can be implemented. > As I pointed out in another mail my > I is to make this work by using file > descriptors for /proc//task/. > I don't want this in the initial patchset though. > I prefer to slowly add those features > once we have gotten the basic functionality > in. > > > > > >Thanks, > >Florian > ```