From: Eric W. Biederman <hidden> Date: 2018-11-30 16:22:09
Arnd Bergmann [off-list ref] writes:
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.
Unfortunately it isn't maintained very well. What you can
express with signalfd_siginfo is a subset what you can express with
siginfo. Many of the linux extensions to siginfo for exception
information add pointers and have integers right after those pointers.
Not all of those linux specific extentions for exceptions are handled
by signalfd_siginfo (it needs new fields).
As originally defined siginfo had the sigval_t union at the end so it
was perfectly fine on both 32bit and 64bit as it only had a single
pointer in the structure and the other fields were 32bits in size.
Although I do feel the pain as x86_64 has to deal with 3 versions
of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
with a 64bit si_clock_t.
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.
If you are not implementing CRIU and reinserting exceptions to yourself.
At most user space wants the ability to implement sigqueue:
AKA:
sigqueue(pid_t pid, int sig, const union sigval value);
Well sigqueue with it's own si_codes so the following would cover all
the use cases I know of:
int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
The si_code could even be set to SI_USER to request that the normal
trusted SI_USER values be filled in. si_code values of < 0 if not
recognized could reasonably safely be treated as the _rt member of
the siginfo union. Or perhaps better we error out in such a case.
If we want to be flexible and not have N kinds of system calls that
is the direction I would go. That is simple, and you don't need any of
the rest.
Alternatively we abandon the mistake of sigqueueinfo and not allow
a signal number in the arguments that differs from the si_signo in the
siginfo and allow passing the entire thing unchanged from sender to
receiver. That is maximum flexibility.
signalfd_siginfo just sucks in practice. It is larger that siginfo 104
bytes versus 48. We must deliver it to userspace as a siginfo so it
must be translated. Because of the translation signalfd_siginfo adds
no flexiblity in practice, because it can not just be passed through.
Finallay signalfd_siginfo does not have encodings for all of the
siginfo union members, so it fails to be fully general.
Personally if I was to define signalfd_siginfo today I would make it:
struct siginfo_subset {
__u32 sis_signo;
__s32 sis_errno;
__s32 sis_code;
__u32 sis_pad;
__u32 sis_pid;
__u32 sis_uid;
__u64 sis_data (A pointer or integer data field);
};
That is just 32bytes, and is all that is needed for everything
except for synchronous exceptions. Oh and that happens to be a proper
subset of a any sane siginfo layout, on both 32bit and 64bit.
This is one of those rare times where POSIX is sane and what linux
has implemented is not.
Eric
From: Christian Brauner <christian@brauner.io> Date: 2018-11-30 18:04:41
On Thu, Nov 29, 2018 at 11:13:57PM -0600, Eric W. Biederman wrote:
Arnd Bergmann [off-list ref] writes:
quoted
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.
Unfortunately it isn't maintained very well. What you can
express with signalfd_siginfo is a subset what you can express with
siginfo. Many of the linux extensions to siginfo for exception
information add pointers and have integers right after those pointers.
Not all of those linux specific extentions for exceptions are handled
by signalfd_siginfo (it needs new fields).
As originally defined siginfo had the sigval_t union at the end so it
was perfectly fine on both 32bit and 64bit as it only had a single
pointer in the structure and the other fields were 32bits in size.
Although I do feel the pain as x86_64 has to deal with 3 versions
of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
with a 64bit si_clock_t.
quoted
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.
If you are not implementing CRIU and reinserting exceptions to yourself.
At most user space wants the ability to implement sigqueue:
AKA:
sigqueue(pid_t pid, int sig, const union sigval value);
Well sigqueue with it's own si_codes so the following would cover all
the use cases I know of:
int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
The si_code could even be set to SI_USER to request that the normal
trusted SI_USER values be filled in. si_code values of < 0 if not
recognized could reasonably safely be treated as the _rt member of
the siginfo union. Or perhaps better we error out in such a case.
If we want to be flexible and not have N kinds of system calls that
is the direction I would go. That is simple, and you don't need any of
the rest.
Alternatively we abandon the mistake of sigqueueinfo and not allow
a signal number in the arguments that differs from the si_signo in the
siginfo and allow passing the entire thing unchanged from sender to
receiver. That is maximum flexibility.
signalfd_siginfo just sucks in practice. It is larger that siginfo 104
bytes versus 48. We must deliver it to userspace as a siginfo so it
must be translated. Because of the translation signalfd_siginfo adds
no flexiblity in practice, because it can not just be passed through.
Finallay signalfd_siginfo does not have encodings for all of the
siginfo union members, so it fails to be fully general.
Personally if I was to define signalfd_siginfo today I would make it:
struct siginfo_subset {
__u32 sis_signo;
__s32 sis_errno;
__s32 sis_code;
__u32 sis_pad;
__u32 sis_pid;
__u32 sis_uid;
__u64 sis_data (A pointer or integer data field);
};
That is just 32bytes, and is all that is needed for everything
except for synchronous exceptions. Oh and that happens to be a proper
subset of a any sane siginfo layout, on both 32bit and 64bit.
This is one of those rare times where POSIX is sane and what linux
has implemented is not.
Thanks for the in-depth explanation. So your point is that we are better
off if we stick with siginfo_t instead of struct signalfd_siginfo in
procfd_signal()?
From: Christian Brauner <christian@brauner.io> Date: 2018-11-30 21:57:11
On December 1, 2018 5:35:45 AM GMT+13:00, Andy Lutomirski [off-list ref] wrote:
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
quoted
siginfo_t as it is now still has a number of other downsides, and
Andy in
quoted
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but
interprets
quoted
it as compat_siginfo depending on
in_compat_syscall()/in_x32_syscall()
quoted
should work correctly, but feels wrong to me, or at least
inconsistent
quoted
with how we do this elsewhere.
If everyone else is okay with it, I can get on board with three
variants on x86. What I can't get on board with is *five* variants on
Thanks Andy, that helps a lot.
I'm okay with it. Does this require any additional changes to how the syscall
is currently hooked up?
x86, which would be:
procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
syscall64 with nr == 335 (presumably): 64-bit
syscall64 with nr == 548 | 0x40000000: x32
syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
true, behavior is arbitrary
syscall64 with nr == 335 | 0x40000000: x32 entry, but
in_compat_syscall() == false, behavior is arbitrary
This mess isn't really Christian's fault -- it's been there for a
while, but it's awful and I don't think we want to perpetuate it.
Obviously, I'd prefer a variant where the structure that's passed in
is always the same.
BTW, do we consider siginfo_t to be extensible? If so, and if we pass
I would prefer if we could consider it extensible.
If so I would prefer if we could pass in a size argument.
in a pointer, presumably we should pass a length as well.
From: Christian Brauner <christian@brauner.io> Date: 2018-11-30 22:26:41
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski [off-list ref]
wrote:
quoted
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
quoted
siginfo_t as it is now still has a number of other downsides, and
Andy in
quoted
quoted
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but
interprets
quoted
quoted
it as compat_siginfo depending on
in_compat_syscall()/in_x32_syscall()
quoted
quoted
should work correctly, but feels wrong to me, or at least
inconsistent
quoted
quoted
with how we do this elsewhere.
If everyone else is okay with it, I can get on board with three
variants on x86. What I can't get on board with is *five* variants
on
quoted
x86, which would be:
procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
syscall64 with nr == 335 (presumably): 64-bit
These seem unavoidable
quoted
syscall64 with nr == 548 | 0x40000000: x32
syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
true, behavior is arbitrary
syscall64 with nr == 335 | 0x40000000: x32 entry, but
in_compat_syscall() == false, behavior is arbitrary
Am I misreading the code? The way I understand it, setting the
0x40000000 bit means that both in_compat_syscall() and
in_x32_syscall become() true, based on
static inline bool in_x32_syscall(void)
{
#ifdef CONFIG_X86_X32_ABI
if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
return true;
#endif
return false;
}
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
quoted
This mess isn't really Christian's fault -- it's been there for a
while, but it's awful and I don't think we want to perpetuate it.
I'm not convinced that not assigning an x32 syscall number
improves the situation, it just means that we now have one
syscall that behaves completely differently from all others,
in that the x32 version requires being called through a
SYSCALL_DEFINE() entry point rather than a
COMPAT_SYSCALL_DEFINE() one, and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
Of course, the nicest option would be to completely remove
x32 so we can stop worrying about it.
One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
However, I do sympathize and understand these concerns.
On Fri, Nov 30, 2018 at 7:56 AM Christian Brauner [off-list ref] wrote:
On Thu, Nov 29, 2018 at 11:13:57PM -0600, Eric W. Biederman wrote:
quoted
Arnd Bergmann [off-list ref] writes:
quoted
On Thu, Nov 29, 2018 at 9:14 PM Andy Lutomirski [off-list ref] wrote:
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.
Unfortunately it isn't maintained very well. What you can
express with signalfd_siginfo is a subset what you can express with
siginfo. Many of the linux extensions to siginfo for exception
information add pointers and have integers right after those pointers.
Not all of those linux specific extentions for exceptions are handled
by signalfd_siginfo (it needs new fields).
Would those fit in the 30 remaining padding bytes?
quoted
As originally defined siginfo had the sigval_t union at the end so it
was perfectly fine on both 32bit and 64bit as it only had a single
pointer in the structure and the other fields were 32bits in size.
The problem with sigval_t of course is that it is incompatible between
32-bit and 64-bit. We can add the same information, but at least on
the syscall level that would have to be a __u64.
quoted
Although I do feel the pain as x86_64 has to deal with 3 versions
of siginfo. A 64bit one. A 32bit one for ia32. A 32bit one for x32
with a 64bit si_clock_t.
At least you and Al have managed to get it down to a single source-level
definition across all architectures, but there is also the (lesser) problem
that the structure has a slightly different binary layout on each of the
classic architectures.
If we go with Andy's suggestion of having only a single binary layout
on x86 for the new call, I'd argue that we should also make it have
the exact same layout on all other architectures.
quoted
quoted
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.
If you are not implementing CRIU and reinserting exceptions to yourself.
At most user space wants the ability to implement sigqueue:
AKA:
sigqueue(pid_t pid, int sig, const union sigval value);
Well sigqueue with it's own si_codes so the following would cover all
the use cases I know of:
int sendnewsig(pid_t pid, int sig, int si_code, const union sigval value);
The si_code could even be set to SI_USER to request that the normal
trusted SI_USER values be filled in. si_code values of < 0 if not
recognized could reasonably safely be treated as the _rt member of
the siginfo union. Or perhaps better we error out in such a case.
If we want to be flexible and not have N kinds of system calls that
is the direction I would go. That is simple, and you don't need any of
the rest.
I'm not sure I understand what you are suggesting here. Would the
low-level implementation of this be based on procfd, or do you
mean that would be done for pid_t at the kernel level, plus another
syscall for procfd?
quoted
Alternatively we abandon the mistake of sigqueueinfo and not allow
a signal number in the arguments that differs from the si_signo in the
siginfo and allow passing the entire thing unchanged from sender to
receiver. That is maximum flexibility.
This would be regardless of which flavor of siginfo (today's arch
specific one, signalfd_siginfo, or a new one) we pass, right?
quoted
signalfd_siginfo just sucks in practice. It is larger that siginfo 104
bytes versus 48. We must deliver it to userspace as a siginfo so it
must be translated. Because of the translation signalfd_siginfo adds
no flexiblity in practice, because it can not just be passed through.
Finallay signalfd_siginfo does not have encodings for all of the
siginfo union members, so it fails to be fully general.
Personally if I was to define signalfd_siginfo today I would make it:
struct siginfo_subset {
__u32 sis_signo;
__s32 sis_errno;
__s32 sis_code;
__u32 sis_pad;
__u32 sis_pid;
__u32 sis_uid;
__u64 sis_data (A pointer or integer data field);
};
That is just 32bytes, and is all that is needed for everything
except for synchronous exceptions. Oh and that happens to be a proper
subset of a any sane siginfo layout, on both 32bit and 64bit.
And that would work for signalfd and waitid_time64, but not for
procfd_signal/kill/sendsiginfo?
quoted
This is one of those rare times where POSIX is sane and what linux
has implemented is not.
Thanks for the in-depth explanation. So your point is that we are better
off if we stick with siginfo_t instead of struct signalfd_siginfo in
procfd_signal()?
I think it means we still need more discussion. Using signalfd_siginfo
without further changes doesn't seem sufficient because of the missing
sigval and the excessive length adds some cost.
siginfo_t as it is now still has a number of other downsides, and Andy in
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but interprets
it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
should work correctly, but feels wrong to me, or at least inconsistent
with how we do this elsewhere.
Arnd
From: Daniel Colascione <hidden> Date: 2018-11-30 23:05:36
On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner [off-list ref] wrote:
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski [off-list ref]
wrote:
quoted
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
quoted
siginfo_t as it is now still has a number of other downsides, and
Andy in
quoted
quoted
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but
interprets
quoted
quoted
it as compat_siginfo depending on
in_compat_syscall()/in_x32_syscall()
quoted
quoted
should work correctly, but feels wrong to me, or at least
inconsistent
quoted
quoted
with how we do this elsewhere.
If everyone else is okay with it, I can get on board with three
variants on x86. What I can't get on board with is *five* variants
on
quoted
x86, which would be:
procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
syscall64 with nr == 335 (presumably): 64-bit
These seem unavoidable
quoted
syscall64 with nr == 548 | 0x40000000: x32
syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
true, behavior is arbitrary
syscall64 with nr == 335 | 0x40000000: x32 entry, but
in_compat_syscall() == false, behavior is arbitrary
Am I misreading the code? The way I understand it, setting the
0x40000000 bit means that both in_compat_syscall() and
in_x32_syscall become() true, based on
static inline bool in_x32_syscall(void)
{
#ifdef CONFIG_X86_X32_ABI
if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
return true;
#endif
return false;
}
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
quoted
This mess isn't really Christian's fault -- it's been there for a
while, but it's awful and I don't think we want to perpetuate it.
I'm not convinced that not assigning an x32 syscall number
improves the situation, it just means that we now have one
syscall that behaves completely differently from all others,
in that the x32 version requires being called through a
SYSCALL_DEFINE() entry point rather than a
COMPAT_SYSCALL_DEFINE() one, and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
Of course, the nicest option would be to completely remove
x32 so we can stop worrying about it.
One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
However, I do sympathize and understand these concerns.
IMHO, it's fine to just replicate all the splits we have for the
existing signal system calls. It's ugly, but once it's done, it'll be
done for a long time. I can't see a need to add even more signal
system calls after this one.
On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione [off-list ref] wrote:
On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner [off-list ref] wrote:
quoted
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
However, I do sympathize and understand these concerns.
IMHO, it's fine to just replicate all the splits we have for the
existing signal system calls. It's ugly, but once it's done, it'll be
done for a long time. I can't see a need to add even more signal
system calls after this one.
We definitely need waitid_time64() and rt_sigtimedwait_time64()
in the very near future.
Arnd
From: Andy Lutomirski <luto@kernel.org> Date: 2018-12-01 03:45:52
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
siginfo_t as it is now still has a number of other downsides, and Andy in
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but interprets
it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
should work correctly, but feels wrong to me, or at least inconsistent
with how we do this elsewhere.
If everyone else is okay with it, I can get on board with three
variants on x86. What I can't get on board with is *five* variants on
x86, which would be:
procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
syscall64 with nr == 335 (presumably): 64-bit
syscall64 with nr == 548 | 0x40000000: x32
syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
true, behavior is arbitrary
syscall64 with nr == 335 | 0x40000000: x32 entry, but
in_compat_syscall() == false, behavior is arbitrary
This mess isn't really Christian's fault -- it's been there for a
while, but it's awful and I don't think we want to perpetuate it.
Obviously, I'd prefer a variant where the structure that's passed in
is always the same.
BTW, do we consider siginfo_t to be extensible? If so, and if we pass
in a pointer, presumably we should pass a length as well.
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski [off-list ref] wrote:
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
quoted
siginfo_t as it is now still has a number of other downsides, and Andy in
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but interprets
it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
should work correctly, but feels wrong to me, or at least inconsistent
with how we do this elsewhere.
If everyone else is okay with it, I can get on board with three
variants on x86. What I can't get on board with is *five* variants on
x86, which would be:
procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
syscall64 with nr == 335 (presumably): 64-bit
These seem unavoidable
syscall64 with nr == 548 | 0x40000000: x32
syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
true, behavior is arbitrary
syscall64 with nr == 335 | 0x40000000: x32 entry, but
in_compat_syscall() == false, behavior is arbitrary
Am I misreading the code? The way I understand it, setting the
0x40000000 bit means that both in_compat_syscall() and
in_x32_syscall become() true, based on
static inline bool in_x32_syscall(void)
{
#ifdef CONFIG_X86_X32_ABI
if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
return true;
#endif
return false;
}
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
This mess isn't really Christian's fault -- it's been there for a
while, but it's awful and I don't think we want to perpetuate it.
I'm not convinced that not assigning an x32 syscall number
improves the situation, it just means that we now have one
syscall that behaves completely differently from all others,
in that the x32 version requires being called through a
SYSCALL_DEFINE() entry point rather than a
COMPAT_SYSCALL_DEFINE() one, and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
Of course, the nicest option would be to completely remove
x32 so we can stop worrying about it.
Arnd
On Sat, Dec 1, 2018 at 12:12 AM Arnd Bergmann [off-list ref] wrote:
On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner [off-list ref] wrote:
quoted
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
One humble point I would like to make is that what I care about most is a sensible way forward without having to redo essential parts of how syscalls work.
I don't want to introduce a sane, small syscall that ends up breaking all over the place because we decided to fix past mistakes that technically have nothing to do with the patch itself.
However, I do sympathize and understand these concerns.
IMHO, it's fine to just replicate all the splits we have for the
existing signal system calls. It's ugly, but once it's done, it'll be
done for a long time. I can't see a need to add even more signal
system calls after this one.
We definitely need waitid_time64() and rt_sigtimedwait_time64()
in the very near future.
To clarify: we probably don't need rt_sigtimedwait_time64() for
x32, as it already has a 64-bit time_t. We might need waitid_time64()
or something similar though, since the plan now is to change the
time resolution for rusage to nanoseconds (__kernel_timespec)
now. The exact behavior and name of waitid_time64() is still
a matter of discussion.
Arnd
From: Christian Brauner <christian@brauner.io> Date: 2018-12-01 10:51:01
On December 1, 2018 12:12:53 PM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione [off-list ref]
wrote:
quoted
On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner
[off-list ref] wrote:
quoted
quoted
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann
[off-list ref] wrote:
quoted
quoted
One humble point I would like to make is that what I care about
most is a sensible way forward without having to redo essential parts
of how syscalls work.
quoted
quoted
I don't want to introduce a sane, small syscall that ends up
breaking all over the place because we decided to fix past mistakes
that technically have nothing to do with the patch itself.
quoted
quoted
However, I do sympathize and understand these concerns.
IMHO, it's fine to just replicate all the splits we have for the
existing signal system calls. It's ugly, but once it's done, it'll be
done for a long time. I can't see a need to add even more signal
system calls after this one.
We definitely need waitid_time64() and rt_sigtimedwait_time64()
in the very near future.
Right, I remember you pointing this out in a prior mail.
Thanks for working on this for such a long time now, Arnd!
Can we agree to move on with the procfd syscall given the current constraints?
I just don't want to see the syscall being
blocked by a generic problem whose
ultimate solution is to get rid of weird
architectural constraints. :)
From: Andy Lutomirski <luto@kernel.org> Date: 2018-12-01 10:57:37
On Fri, Nov 30, 2018 at 3:40 PM Christian Brauner [off-list ref] wrote:
On December 1, 2018 12:12:53 PM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
quoted
On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione [off-list ref]
wrote:
quoted
On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner
[off-list ref] wrote:
quoted
quoted
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann
[off-list ref] wrote:
quoted
quoted
One humble point I would like to make is that what I care about
most is a sensible way forward without having to redo essential parts
of how syscalls work.
quoted
quoted
I don't want to introduce a sane, small syscall that ends up
breaking all over the place because we decided to fix past mistakes
that technically have nothing to do with the patch itself.
quoted
quoted
However, I do sympathize and understand these concerns.
IMHO, it's fine to just replicate all the splits we have for the
existing signal system calls. It's ugly, but once it's done, it'll be
done for a long time. I can't see a need to add even more signal
system calls after this one.
We definitely need waitid_time64() and rt_sigtimedwait_time64()
in the very near future.
Right, I remember you pointing this out in a prior mail.
Thanks for working on this for such a long time now, Arnd!
Can we agree to move on with the procfd syscall given the current constraints?
I just don't want to see the syscall being
blocked by a generic problem whose
ultimate solution is to get rid of weird
architectural constraints.
Creating and using a copy_siginfo_from_user64() function would work
for everyone, no?
From: Andy Lutomirski <luto@kernel.org> Date: 2018-12-01 11:05:00
On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann [off-list ref] wrote:
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
quoted
siginfo_t as it is now still has a number of other downsides, and Andy in
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but interprets
it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
should work correctly, but feels wrong to me, or at least inconsistent
with how we do this elsewhere.
If everyone else is okay with it, I can get on board with three
variants on x86. What I can't get on board with is *five* variants on
x86, which would be:
procfd_signal via int80 / the 32-bit vDSO: the ia32 structure
syscall64 with nr == 335 (presumably): 64-bit
These seem unavoidable
Indeed, although, in hindsight, they should have had the same numbers.
quoted
syscall64 with nr == 548 | 0x40000000: x32
syscall64 with nr == 548: 64-bit entry but in_compat_syscall() ==
true, behavior is arbitrary
syscall64 with nr == 335 | 0x40000000: x32 entry, but
in_compat_syscall() == false, behavior is arbitrary
Am I misreading the code? The way I understand it, setting the
0x40000000 bit means that both in_compat_syscall() and
in_x32_syscall become() true, based on
static inline bool in_x32_syscall(void)
{
#ifdef CONFIG_X86_X32_ABI
if (task_pt_regs(current)->orig_ax & __X32_SYSCALL_BIT)
return true;
#endif
return false;
}
Yes.
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
I would propose that it should be 335 | 0x40000000. I can't see any
reasonable way to teach the kernel to reject 335 | 0x40000000 that
wouldn't work just as well to accept it and make it do the right
thing. Currently we accept it and do the *wrong* thing, which is no
good.
quoted
This mess isn't really Christian's fault -- it's been there for a
while, but it's awful and I don't think we want to perpetuate it.
I'm not convinced that not assigning an x32 syscall number
improves the situation, it just means that we now have one
syscall that behaves completely differently from all others,
in that the x32 version requires being called through a
SYSCALL_DEFINE() entry point rather than a
COMPAT_SYSCALL_DEFINE() one,
There's nothing particularly novel about this. seccomp(), for
example, already works like this.
and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
What hack are you referring to here?
Of course, the nicest option would be to completely remove
x32 so we can stop worrying about it.
From: Christian Brauner <christian@brauner.io> Date: 2018-12-01 12:32:02
On December 1, 2018 12:46:22 PM GMT+13:00, Andy Lutomirski [off-list ref] wrote:
On Fri, Nov 30, 2018 at 3:40 PM Christian Brauner
[off-list ref] wrote:
quoted
On December 1, 2018 12:12:53 PM GMT+13:00, Arnd Bergmann
[off-list ref] wrote:
quoted
quoted
On Sat, Dec 1, 2018 at 12:05 AM Daniel Colascione
[off-list ref]
quoted
quoted
wrote:
quoted
On Fri, Nov 30, 2018 at 2:26 PM Christian Brauner
[off-list ref] wrote:
quoted
quoted
On December 1, 2018 11:09:58 AM GMT+13:00, Arnd Bergmann
[off-list ref] wrote:
quoted
quoted
One humble point I would like to make is that what I care about
most is a sensible way forward without having to redo essential
parts
quoted
quoted
of how syscalls work.
quoted
quoted
I don't want to introduce a sane, small syscall that ends up
breaking all over the place because we decided to fix past mistakes
that technically have nothing to do with the patch itself.
quoted
quoted
However, I do sympathize and understand these concerns.
IMHO, it's fine to just replicate all the splits we have for the
existing signal system calls. It's ugly, but once it's done, it'll
be
quoted
quoted
quoted
done for a long time. I can't see a need to add even more signal
system calls after this one.
We definitely need waitid_time64() and rt_sigtimedwait_time64()
in the very near future.
Right, I remember you pointing this out in a prior mail.
Thanks for working on this for such a long time now, Arnd!
Can we agree to move on with the procfd syscall given the current
constraints?
quoted
I just don't want to see the syscall being
blocked by a generic problem whose
ultimate solution is to get rid of weird
architectural constraints.
Creating and using a copy_siginfo_from_user64() function would work
for everyone, no?
Meaning, no compat syscalls, introduce
new struct siginfo64_t and the copy
function you named above?
On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski [off-list ref] wrote:
On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
quoted
siginfo_t as it is now still has a number of other downsides, and Andy in
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but interprets
it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
should work correctly, but feels wrong to me, or at least inconsistent
with how we do this elsewhere.
quoted
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
I would propose that it should be 335 | 0x40000000. I can't see any
reasonable way to teach the kernel to reject 335 | 0x40000000 that
wouldn't work just as well to accept it and make it do the right
thing. Currently we accept it and do the *wrong* thing, which is no
good.
quoted
and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
What hack are you referring to here?
I mean this part:
#ifdef CONFIG_COMPAT
int copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from)
#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION)
{
return __copy_siginfo_to_user32(to, from, in_x32_syscall());
}
int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from, bool x32_ABI)
#endif
{
...
case SIL_CHLD:
new.si_pid = from->si_pid;
new.si_uid = from->si_uid;
new.si_status = from->si_status;
#ifdef CONFIG_X86_X32_ABI
if (x32_ABI) {
new._sifields._sigchld_x32._utime = from->si_utime;
new._sifields._sigchld_x32._stime = from->si_stime;
} else
#endif
{
new.si_utime = from->si_utime;
new.si_stime = from->si_stime;
}
break;
...
}
#endif
If we have a '548 | 0x40000000' entry pointing to
__x32_compat_sys_procfd_kill, then that will do the right
thing. If you instead try to have x32 call into the native
sys_procfd_kill, then copy_siginfo_to_user() will also have
to know about x32, effectively duplicating that mess above,
unless you want to also change all users of
copy_siginfo_to_user32() to use copy_siginfo_to_user()
and handle all cases in one function.
Arnd
From: Christian Brauner <christian@brauner.io> Date: 2018-12-01 20:29:56
On December 1, 2018 9:51:18 PM GMT+13:00, Arnd Bergmann [off-list ref] wrote:
On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski [off-list ref]
wrote:
quoted
On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski [off-list ref]
wrote:
quoted
quoted
quoted
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref]
wrote:
quoted
quoted
quoted
quoted
siginfo_t as it is now still has a number of other downsides,
and Andy in
quoted
quoted
quoted
quoted
particular didn't like the idea of having three new variants on
x86
quoted
quoted
quoted
quoted
(depending on how you count). His alternative suggestion of
having
quoted
quoted
quoted
quoted
a single syscall entry point that takes a 'signfo_t __user *'
but interprets
quoted
quoted
quoted
quoted
it as compat_siginfo depending on
in_compat_syscall()/in_x32_syscall()
quoted
quoted
quoted
quoted
should work correctly, but feels wrong to me, or at least
inconsistent
quoted
quoted
quoted
quoted
with how we do this elsewhere.
quoted
quoted
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
I would propose that it should be 335 | 0x40000000. I can't see any
reasonable way to teach the kernel to reject 335 | 0x40000000 that
wouldn't work just as well to accept it and make it do the right
thing. Currently we accept it and do the *wrong* thing, which is no
good.
quoted
and we have to
add more complexity to the copy_siginfo_from_user()
implementation to duplicate the hack that exists in
copy_siginfo_from_user32().
What hack are you referring to here?
I mean this part:
#ifdef CONFIG_COMPAT
int copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from)
#if defined(CONFIG_X86_X32_ABI) || defined(CONFIG_IA32_EMULATION)
{
return __copy_siginfo_to_user32(to, from, in_x32_syscall());
}
int __copy_siginfo_to_user32(struct compat_siginfo __user *to,
const struct kernel_siginfo *from, bool x32_ABI)
#endif
{
...
case SIL_CHLD:
new.si_pid = from->si_pid;
new.si_uid = from->si_uid;
new.si_status = from->si_status;
#ifdef CONFIG_X86_X32_ABI
if (x32_ABI) {
new._sifields._sigchld_x32._utime = from->si_utime;
new._sifields._sigchld_x32._stime = from->si_stime;
} else
#endif
{
new.si_utime = from->si_utime;
new.si_stime = from->si_stime;
}
break;
...
}
#endif
If we have a '548 | 0x40000000' entry pointing to
__x32_compat_sys_procfd_kill, then that will do the right
thing. If you instead try to have x32 call into the native
sys_procfd_kill, then copy_siginfo_to_user() will also have
to know about x32, effectively duplicating that mess above,
unless you want to also change all users of
copy_siginfo_to_user32() to use copy_siginfo_to_user()
and handle all cases in one function.
I've been looking into having siginfo64_t
with the new copy_siginfo_to_user64()
function. It looks like a pretty intricate
task. Are we sure that we want to go
down this road? I'm not sure that it'll be
worth it. Especially since we force yet
another signal struct on user space.
On Sat, Dec 1, 2018 at 9:51 AM Arnd Bergmann [off-list ref] wrote:
On Sat, Dec 1, 2018 at 12:54 AM Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 2:10 PM Arnd Bergmann [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 5:36 PM Andy Lutomirski [off-list ref] wrote:
quoted
On Fri, Nov 30, 2018 at 3:41 AM Arnd Bergmann [off-list ref] wrote:
quoted
siginfo_t as it is now still has a number of other downsides, and Andy in
particular didn't like the idea of having three new variants on x86
(depending on how you count). His alternative suggestion of having
a single syscall entry point that takes a 'signfo_t __user *' but interprets
it as compat_siginfo depending on in_compat_syscall()/in_x32_syscall()
should work correctly, but feels wrong to me, or at least inconsistent
with how we do this elsewhere.
quoted
quoted
The '548 | 0x40000000' part seems to be the only sensible
way to handle x32 here. What exactly would you propose to
avoid defining the other entry points?
I would propose that it should be 335 | 0x40000000. I can't see any
reasonable way to teach the kernel to reject 335 | 0x40000000 that
wouldn't work just as well to accept it and make it do the right
thing. Currently we accept it and do the *wrong* thing, which is no
good.
I guess we could start with something like the change below, which
would unify the entry points for rt_{tg,}sigqueueinfo, so that
e.g. the 129 and 536 syscall numbers do the exact same thing, and
that would be the lp64 or ilp32 behavior, depending on the
0x40000000 bit. For the new syscalls, we can then do the same
thing without assigning another number.
Arnd
@@ -7,6 +7,11 @@#include<asm/asm-offsets.h>#include<asm/syscall.h>+#ifdef CONFIG_X86_X32_ABI+#define __x64_sys_x86_rt_sigqueueinfo __x64_sys_rt_sigqueueinfo+#define __x64_sys_x86_rt_tgsigqueueinfo __x64_sys_rt_tgsigqueueinfo+#endif+/* this is a lie, but it does not hurt as sys_ni_syscall just returns-EINVAL*/externasmlinkagelongsys_ni_syscall(conststructpt_regs*);#define __SYSCALL_64(nr, sym, qual) extern asmlinkage long sym(const