Re: [PATCH 7/3] signal: Deliver all of the perf_data in si_perf

Marco Elver [off-list ref] writes:

On Sat, 1 May 2021 at 01:44, Eric W. Biederman [off-list ref] wrote:

quoted

Don't abuse si_errno and deliver all of the perf data in si_perf.

Signed-off-by: "Eric W. Biederman" <redacted>
---

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:

quoted

 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

quoted

 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.

quoted

+                       } _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 [off-list ref] wrote:

Marco Elver [off-list ref] writes:

quoted

On Sat, 1 May 2021 at 01:44, Eric W. Biederman [off-list ref] wrote:

quoted

Don't abuse si_errno and deliver all of the perf data in si_perf.

Signed-off-by: "Eric W. Biederman" <redacted>
---

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:

quoted

 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.