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

77 more quoted lines

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.