| Message ID | m11rarqqx2.fsf_-_@fess.ebiederm.org |
|---|---|
| State | New |
| Headers | show |
| Series | signal: Move si_trapno into the _si_fault union | expand |
On Sat, 1 May 2021 at 01:44, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Don't abuse si_errno and deliver all of the perf data in si_perf. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- Thank you for the fix, this looks cleaner. Just note that this patch needs to include updates to tools/testing/selftests/perf_events. This should do it: > sed -i 's/si_perf/si_perf.data/g; s/si_errno/si_perf.type/g' tools/testing/selftests/perf_events/*.c Subject: s/perf_data/perf data/ ? For uapi, need to switch to __u32, see below. > fs/signalfd.c | 3 ++- > include/linux/compat.h | 5 ++++- > include/uapi/asm-generic/siginfo.h | 5 ++++- > include/uapi/linux/signalfd.h | 4 ++-- > kernel/signal.c | 18 +++++++++++------- > 5 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/fs/signalfd.c b/fs/signalfd.c > index 83130244f653..9686af56f073 100644 > --- a/fs/signalfd.c > +++ b/fs/signalfd.c > @@ -134,7 +134,8 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, > break; > case SIL_FAULT_PERF_EVENT: > new.ssi_addr = (long) kinfo->si_addr; > - new.ssi_perf = kinfo->si_perf; > + new.ssi_perf_type = kinfo->si_perf.type; > + new.ssi_perf_data = kinfo->si_perf.data; > break; > case SIL_CHLD: > new.ssi_pid = kinfo->si_pid; > diff --git a/include/linux/compat.h b/include/linux/compat.h > index 24462ed63af4..0726f9b3a57c 100644 > --- a/include/linux/compat.h > +++ b/include/linux/compat.h > @@ -235,7 +235,10 @@ typedef struct compat_siginfo { > u32 _pkey; > } _addr_pkey; > /* used when si_code=TRAP_PERF */ > - compat_ulong_t _perf; > + struct { > + compat_ulong_t data; > + u32 type; > + } _perf; > }; > } _sigfault; > > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > index 2abdf1d19aad..19b6310021a3 100644 > --- a/include/uapi/asm-generic/siginfo.h > +++ b/include/uapi/asm-generic/siginfo.h > @@ -90,7 +90,10 @@ union __sifields { > __u32 _pkey; > } _addr_pkey; > /* used when si_code=TRAP_PERF */ > - unsigned long _perf; > + struct { > + unsigned long data; > + u32 type; This needs to be __u32. > + } _perf; > }; > } _sigfault; > > diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h > index 7e333042c7e3..e78dddf433fc 100644 > --- a/include/uapi/linux/signalfd.h > +++ b/include/uapi/linux/signalfd.h > @@ -39,8 +39,8 @@ struct signalfd_siginfo { > __s32 ssi_syscall; > __u64 ssi_call_addr; > __u32 ssi_arch; > - __u32 __pad3; > - __u64 ssi_perf; > + __u32 ssi_perf_type; > + __u64 ssi_perf_data; > > /* > * Pad strcture to 128 bytes. Remember to update the > diff --git a/kernel/signal.c b/kernel/signal.c > index 5b1ad7f080ab..cb3574b7319c 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -1758,11 +1758,13 @@ int force_sig_perf(void __user *pending_addr, u32 type, u64 sig_data) > struct kernel_siginfo info; > > clear_siginfo(&info); > - info.si_signo = SIGTRAP; > - info.si_errno = type; > - info.si_code = TRAP_PERF; > - info.si_addr = pending_addr; > - info.si_perf = sig_data; > + info.si_signo = SIGTRAP; > + info.si_errno = 0; > + info.si_code = TRAP_PERF; > + info.si_addr = pending_addr; > + info.si_perf.data = sig_data; > + info.si_perf.type = type; > + > return force_sig_info(&info); > } > > @@ -3379,7 +3381,8 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, > break; > case SIL_FAULT_PERF_EVENT: > to->si_addr = ptr_to_compat(from->si_addr); > - to->si_perf = from->si_perf; > + to->si_perf.data = from->si_perf.data; > + to->si_perf.type = from->si_perf.type; > break; > case SIL_CHLD: > to->si_pid = from->si_pid; > @@ -3455,7 +3458,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, > break; > case SIL_FAULT_PERF_EVENT: > to->si_addr = compat_ptr(from->si_addr); > - to->si_perf = from->si_perf; > + to->si_perf.data = from->si_perf.data; > + to->si_perf.type = from->si_perf.type; > break; > case SIL_CHLD: > to->si_pid = from->si_pid; > -- > 2.30.1
Marco Elver <elver@google.com> writes: > On Sat, 1 May 2021 at 01:44, Eric W. Biederman <ebiederm@xmission.com> wrote: >> >> Don't abuse si_errno and deliver all of the perf data in si_perf. >> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- > > Thank you for the fix, this looks cleaner. > > Just note that this patch needs to include updates to > tools/testing/selftests/perf_events. This should do it: >> sed -i 's/si_perf/si_perf.data/g; s/si_errno/si_perf.type/g' tools/testing/selftests/perf_events/*.c > > Subject: s/perf_data/perf data/ ? > > For uapi, need to switch to __u32, see below. Good point. The one thing that this doesn't do is give you a 64bit field on 32bit architectures. On 32bit builds the layout is: int si_signo; int si_errno; int si_code; void __user *_addr; So I believe if the first 3 fields were moved into the _sifields union si_perf could define a 64bit field as it's first member and it would not break anything else. Given that the data field is 64bit that seems desirable. Eric >> fs/signalfd.c | 3 ++- >> include/linux/compat.h | 5 ++++- >> include/uapi/asm-generic/siginfo.h | 5 ++++- >> include/uapi/linux/signalfd.h | 4 ++-- >> kernel/signal.c | 18 +++++++++++------- >> 5 files changed, 23 insertions(+), 12 deletions(-) >> >> diff --git a/fs/signalfd.c b/fs/signalfd.c >> index 83130244f653..9686af56f073 100644 >> --- a/fs/signalfd.c >> +++ b/fs/signalfd.c >> @@ -134,7 +134,8 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, >> break; >> case SIL_FAULT_PERF_EVENT: >> new.ssi_addr = (long) kinfo->si_addr; >> - new.ssi_perf = kinfo->si_perf; >> + new.ssi_perf_type = kinfo->si_perf.type; >> + new.ssi_perf_data = kinfo->si_perf.data; >> break; >> case SIL_CHLD: >> new.ssi_pid = kinfo->si_pid; >> diff --git a/include/linux/compat.h b/include/linux/compat.h >> index 24462ed63af4..0726f9b3a57c 100644 >> --- a/include/linux/compat.h >> +++ b/include/linux/compat.h >> @@ -235,7 +235,10 @@ typedef struct compat_siginfo { >> u32 _pkey; >> } _addr_pkey; >> /* used when si_code=TRAP_PERF */ >> - compat_ulong_t _perf; >> + struct { >> + compat_ulong_t data; >> + u32 type; >> + } _perf; >> }; >> } _sigfault; >> >> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h >> index 2abdf1d19aad..19b6310021a3 100644 >> --- a/include/uapi/asm-generic/siginfo.h >> +++ b/include/uapi/asm-generic/siginfo.h >> @@ -90,7 +90,10 @@ union __sifields { >> __u32 _pkey; >> } _addr_pkey; >> /* used when si_code=TRAP_PERF */ >> - unsigned long _perf; >> + struct { >> + unsigned long data; >> + u32 type; > > This needs to be __u32. > > >> + } _perf; >> }; >> } _sigfault; >> >> diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h >> index 7e333042c7e3..e78dddf433fc 100644 >> --- a/include/uapi/linux/signalfd.h >> +++ b/include/uapi/linux/signalfd.h >> @@ -39,8 +39,8 @@ struct signalfd_siginfo { >> __s32 ssi_syscall; >> __u64 ssi_call_addr; >> __u32 ssi_arch; >> - __u32 __pad3; >> - __u64 ssi_perf; >> + __u32 ssi_perf_type; >> + __u64 ssi_perf_data; >> >> /* >> * Pad strcture to 128 bytes. Remember to update the >> diff --git a/kernel/signal.c b/kernel/signal.c >> index 5b1ad7f080ab..cb3574b7319c 100644 >> --- a/kernel/signal.c >> +++ b/kernel/signal.c >> @@ -1758,11 +1758,13 @@ int force_sig_perf(void __user *pending_addr, u32 type, u64 sig_data) >> struct kernel_siginfo info; >> >> clear_siginfo(&info); >> - info.si_signo = SIGTRAP; >> - info.si_errno = type; >> - info.si_code = TRAP_PERF; >> - info.si_addr = pending_addr; >> - info.si_perf = sig_data; >> + info.si_signo = SIGTRAP; >> + info.si_errno = 0; >> + info.si_code = TRAP_PERF; >> + info.si_addr = pending_addr; >> + info.si_perf.data = sig_data; >> + info.si_perf.type = type; >> + >> return force_sig_info(&info); >> } >> >> @@ -3379,7 +3381,8 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, >> break; >> case SIL_FAULT_PERF_EVENT: >> to->si_addr = ptr_to_compat(from->si_addr); >> - to->si_perf = from->si_perf; >> + to->si_perf.data = from->si_perf.data; >> + to->si_perf.type = from->si_perf.type; >> break; >> case SIL_CHLD: >> to->si_pid = from->si_pid; >> @@ -3455,7 +3458,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, >> break; >> case SIL_FAULT_PERF_EVENT: >> to->si_addr = compat_ptr(from->si_addr); >> - to->si_perf = from->si_perf; >> + to->si_perf.data = from->si_perf.data; >> + to->si_perf.type = from->si_perf.type; >> break; >> case SIL_CHLD: >> to->si_pid = from->si_pid; >> -- >> 2.30.1
On Sun, 2 May 2021 at 20:39, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Marco Elver <elver@google.com> writes: > > > On Sat, 1 May 2021 at 01:44, Eric W. Biederman <ebiederm@xmission.com> wrote: > >> > >> Don't abuse si_errno and deliver all of the perf data in si_perf. > >> > >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > >> --- > > > > Thank you for the fix, this looks cleaner. > > > > Just note that this patch needs to include updates to > > tools/testing/selftests/perf_events. This should do it: > >> sed -i 's/si_perf/si_perf.data/g; s/si_errno/si_perf.type/g' tools/testing/selftests/perf_events/*.c > > > > Subject: s/perf_data/perf data/ ? > > > > For uapi, need to switch to __u32, see below. > > Good point. > > The one thing that this doesn't do is give you a 64bit field > on 32bit architectures. > > On 32bit builds the layout is: > > int si_signo; > int si_errno; > int si_code; > void __user *_addr; > > So I believe if the first 3 fields were moved into the _sifields union > si_perf could define a 64bit field as it's first member and it would not > break anything else. > > Given that the data field is 64bit that seems desirable. Yes, it's quite unfortunate -- it was __u64 at first, but then we noticed it broke 32-bit architectures like arm: https://lore.kernel.org/linux-arch/20210422191823.79012-1-elver@google.com/ Thanks, -- Marco
On Sun, May 02, 2021 at 01:39:16PM -0500, Eric W. Biederman wrote: > The one thing that this doesn't do is give you a 64bit field > on 32bit architectures. > > On 32bit builds the layout is: > > int si_signo; > int si_errno; > int si_code; > void __user *_addr; > > So I believe if the first 3 fields were moved into the _sifields union > si_perf could define a 64bit field as it's first member and it would not > break anything else. > > Given that the data field is 64bit that seems desirable. The data field is fundamentally an address, it is internally a u64 because the perf ring buffer has u64 alignment and it saves on compat crap etc. So for the 32bit/compat case the high bits will always be 0 and truncating into an unsigned long is fine.
Peter Zijlstra <peterz@infradead.org> writes: > On Sun, May 02, 2021 at 01:39:16PM -0500, Eric W. Biederman wrote: > >> The one thing that this doesn't do is give you a 64bit field >> on 32bit architectures. >> >> On 32bit builds the layout is: >> >> int si_signo; >> int si_errno; >> int si_code; >> void __user *_addr; >> >> So I believe if the first 3 fields were moved into the _sifields union >> si_perf could define a 64bit field as it's first member and it would not >> break anything else. >> >> Given that the data field is 64bit that seems desirable. > > The data field is fundamentally an address, it is internally a u64 > because the perf ring buffer has u64 alignment and it saves on compat > crap etc. > > So for the 32bit/compat case the high bits will always be 0 and > truncating into an unsigned long is fine. I see why it is fine to truncate the data field into an unsigned long. Other than technical difficulties in extending siginfo_t is there any reason not to define data as a __u64? Eric
On Mon, 3 May 2021 at 21:38, Eric W. Biederman <ebiederm@xmission.com> wrote: > Peter Zijlstra <peterz@infradead.org> writes: > > > On Sun, May 02, 2021 at 01:39:16PM -0500, Eric W. Biederman wrote: > > > >> The one thing that this doesn't do is give you a 64bit field > >> on 32bit architectures. > >> > >> On 32bit builds the layout is: > >> > >> int si_signo; > >> int si_errno; > >> int si_code; > >> void __user *_addr; > >> > >> So I believe if the first 3 fields were moved into the _sifields union > >> si_perf could define a 64bit field as it's first member and it would not > >> break anything else. > >> > >> Given that the data field is 64bit that seems desirable. > > > > The data field is fundamentally an address, it is internally a u64 > > because the perf ring buffer has u64 alignment and it saves on compat > > crap etc. > > > > So for the 32bit/compat case the high bits will always be 0 and > > truncating into an unsigned long is fine. > > I see why it is fine to truncate the data field into an unsigned long. > > Other than technical difficulties in extending siginfo_t is there any > reason not to define data as a __u64? No -- like I pointed at earlier, si_perf used to be __u64, but we can't because of the siginfo_t limitation. What we have now is fine, and not worth dwelling over given siginfo limitations. Thanks, -- Marco
diff --git a/fs/signalfd.c b/fs/signalfd.c index 83130244f653..9686af56f073 100644 --- a/fs/signalfd.c +++ b/fs/signalfd.c @@ -134,7 +134,8 @@ static int signalfd_copyinfo(struct signalfd_siginfo __user *uinfo, break; case SIL_FAULT_PERF_EVENT: new.ssi_addr = (long) kinfo->si_addr; - new.ssi_perf = kinfo->si_perf; + new.ssi_perf_type = kinfo->si_perf.type; + new.ssi_perf_data = kinfo->si_perf.data; break; case SIL_CHLD: new.ssi_pid = kinfo->si_pid; diff --git a/include/linux/compat.h b/include/linux/compat.h index 24462ed63af4..0726f9b3a57c 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -235,7 +235,10 @@ typedef struct compat_siginfo { u32 _pkey; } _addr_pkey; /* used when si_code=TRAP_PERF */ - compat_ulong_t _perf; + struct { + compat_ulong_t data; + u32 type; + } _perf; }; } _sigfault; diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h index 2abdf1d19aad..19b6310021a3 100644 --- a/include/uapi/asm-generic/siginfo.h +++ b/include/uapi/asm-generic/siginfo.h @@ -90,7 +90,10 @@ union __sifields { __u32 _pkey; } _addr_pkey; /* used when si_code=TRAP_PERF */ - unsigned long _perf; + struct { + unsigned long data; + u32 type; + } _perf; }; } _sigfault; diff --git a/include/uapi/linux/signalfd.h b/include/uapi/linux/signalfd.h index 7e333042c7e3..e78dddf433fc 100644 --- a/include/uapi/linux/signalfd.h +++ b/include/uapi/linux/signalfd.h @@ -39,8 +39,8 @@ struct signalfd_siginfo { __s32 ssi_syscall; __u64 ssi_call_addr; __u32 ssi_arch; - __u32 __pad3; - __u64 ssi_perf; + __u32 ssi_perf_type; + __u64 ssi_perf_data; /* * Pad strcture to 128 bytes. Remember to update the diff --git a/kernel/signal.c b/kernel/signal.c index 5b1ad7f080ab..cb3574b7319c 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -1758,11 +1758,13 @@ int force_sig_perf(void __user *pending_addr, u32 type, u64 sig_data) struct kernel_siginfo info; clear_siginfo(&info); - info.si_signo = SIGTRAP; - info.si_errno = type; - info.si_code = TRAP_PERF; - info.si_addr = pending_addr; - info.si_perf = sig_data; + info.si_signo = SIGTRAP; + info.si_errno = 0; + info.si_code = TRAP_PERF; + info.si_addr = pending_addr; + info.si_perf.data = sig_data; + info.si_perf.type = type; + return force_sig_info(&info); } @@ -3379,7 +3381,8 @@ void copy_siginfo_to_external32(struct compat_siginfo *to, break; case SIL_FAULT_PERF_EVENT: to->si_addr = ptr_to_compat(from->si_addr); - to->si_perf = from->si_perf; + to->si_perf.data = from->si_perf.data; + to->si_perf.type = from->si_perf.type; break; case SIL_CHLD: to->si_pid = from->si_pid; @@ -3455,7 +3458,8 @@ static int post_copy_siginfo_from_user32(kernel_siginfo_t *to, break; case SIL_FAULT_PERF_EVENT: to->si_addr = compat_ptr(from->si_addr); - to->si_perf = from->si_perf; + to->si_perf.data = from->si_perf.data; + to->si_perf.type = from->si_perf.type; break; case SIL_CHLD: to->si_pid = from->si_pid;
Don't abuse si_errno and deliver all of the perf data in si_perf. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/signalfd.c | 3 ++- include/linux/compat.h | 5 ++++- include/uapi/asm-generic/siginfo.h | 5 ++++- include/uapi/linux/signalfd.h | 4 ++-- kernel/signal.c | 18 +++++++++++------- 5 files changed, 23 insertions(+), 12 deletions(-)