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
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
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.
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
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.
From: Andy Lutomirski <luto@amacapital.net> Date: 2018-11-30 04:00:44
On Nov 29, 2018, at 4:28 AM, Florian Weimer [off-list ref] wrote:
Disclaimer: I'm looking at this patch because Christian requested it.
I'm not a kernel developer.
* Christian Brauner:
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
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.
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
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.
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
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?
On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski [off-list ref] wrote:
quoted
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:
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
From: Christian Brauner <christian@brauner.io> Date: 2018-11-30 08:42:00
On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote:
On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski [off-list ref] wrote:
quoted
quoted
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:
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_<whatever-suffix>? 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/<pid>/task/<tid>
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/<pid> and /proc/<pid>/task/<tid> 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
On Thu, Nov 29, 2018 at 10:35 PM Christian Brauner [off-list ref] wrote:
On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote:
quoted
On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski [off-list 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/<pid>/task/<tid>
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/<pid> and /proc/<pid>/task/<tid> 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.
quoted
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
On 2018-11-29, Arnd Bergmann [off-list 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
<https://www.cyphar.com/>
From: Christian Brauner <christian@brauner.io> Date: 2018-12-01 11:03:40
On November 30, 2018 1:28:15 AM GMT+13:00, Florian Weimer [off-list 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.
Is there a reason why these numbers have to be different?
(See the recent discussion with Andy Lutomirski.)
quoted
+static int do_procfd_signal(int fd, int sig, kernel_siginfo_t
*kinfo, int flags,
quoted
+ 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;
[…]
quoted
+ 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.
quoted
+/**
+ * sys_procfd_signal - send a signal to a process through a process
file
quoted
+ * 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,
quoted
+ 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/<pid>/task/<tid>.
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.
From: Christian Brauner <christian@brauner.io> Date: 2018-12-01 12:36:49
On November 30, 2018 10:40:49 AM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
On Thu, Nov 29, 2018 at 10:35 PM Christian Brauner
[off-list ref] wrote:
quoted
On Thu, Nov 29, 2018 at 10:02:13PM +0100, Arnd Bergmann wrote:
quoted
On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski
[off-list ref] wrote:
quoted
quoted
Is the current procfd_signal() proposal (under whichever name)
sufficient
quoted
quoted
to correctly implement both sys_rt_sigqueueinfo() and
sys_rt_tgsigqueueinfo()?
quoted
Yes, I see no reason why not. My idea is to extend it - after we have
a
quoted
basic version in - to also work with:
/proc/<pid>/task/<tid>
If I'm not mistaken this should be sufficient to get
rt_tgsigqueueinfo.
quoted
The thread will be uniquely identified by the tid descriptor and no
combination of /proc/<pid> and /proc/<pid>/task/<tid> 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.
quoted
quoted
Can we implement sys_rt_sigtimedwait() based on signalfd()?
If yes, that would leave waitid(), which already needs a
replacement
quoted
quoted
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
quoted
quoted
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
From: Christian Brauner <christian@brauner.io> Date: 2018-12-02 10:03:17
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 [off-list ref] wrote:
quoted
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.
Is there a reason why these numbers have to be different?
(See the recent discussion with Andy Lutomirski.)
quoted
+static int do_procfd_signal(int fd, int sig, kernel_siginfo_t
*kinfo, int flags,
quoted
+ 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;
[…]
quoted
+ 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/<pid> will still be around
which means that struct pid must still be around.
quoted
quoted
+/**
+ * sys_procfd_signal - send a signal to a process through a process
file
quoted
+ * 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,
quoted
+ 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.
quoted
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/<pid>/task/<tid>.
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.