perf: Copy perf_event_attr::sig_data on modification

Test that PERF_EVENT_IOC_MODIFY_ATTRIBUTES correctly modifies perf_event_attr::sig_data as well.

Signed-off-by Marco Elver <[email protected]>

---
 .../selftests/perf_events/sigtrap_threads.c     | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

tools/testing/selftests/perf_events/sigtrap_threads.c +9 -9

index 8e83cf91513a..6d849dc2bee0 100644
@@ -44,9 +44,10 @@ static struct {
 } ctx;
 
 /* Unique value to check si_perf_data is correctly set from perf_event_attr::sig_data. */
-#define TEST_SIG_DATA(addr) (~(unsigned long)(addr))
+#define TEST_SIG_DATA(addr, id) (~(unsigned long)(addr) + id)
 
-static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr)
+static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr,
+                                             unsigned long id)
 {
        struct perf_event_attr attr = {
                .type           = PERF_TYPE_BREAKPOINT,

@@ -60,7 +61,7 @@ static struct perf_event_attr make_event_attr(bool enabled, volatile void *addr)
                .inherit_thread = 1, /* ... but only cloned with CLONE_THREAD. */
                .remove_on_exec = 1, /* Required by sigtrap. */
                .sigtrap        = 1, /* Request synchronous SIGTRAP on event. */
-               .sig_data       = TEST_SIG_DATA(addr),
+               .sig_data       = TEST_SIG_DATA(addr, id),
        };
        return attr;
 }

@@ -110,7 +111,7 @@ FIXTURE(sigtrap_threads)
 
 FIXTURE_SETUP(sigtrap_threads)
 {
-       struct perf_event_attr attr = make_event_attr(false, &ctx.iterate_on);
+       struct perf_event_attr attr = make_event_attr(false, &ctx.iterate_on, 0);
        struct sigaction action = {};
        int i;
 

@@ -165,7 +166,7 @@ TEST_F(sigtrap_threads, enable_event)
        EXPECT_EQ(ctx.tids_want_signal, 0);
        EXPECT_EQ(ctx.first_siginfo.si_addr, &ctx.iterate_on);
        EXPECT_EQ(ctx.first_siginfo.si_perf_type, PERF_TYPE_BREAKPOINT);
-       EXPECT_EQ(ctx.first_siginfo.si_perf_data, TEST_SIG_DATA(&ctx.iterate_on));
+       EXPECT_EQ(ctx.first_siginfo.si_perf_data, TEST_SIG_DATA(&ctx.iterate_on, 0));
 
        /* Check enabled for parent. */
        ctx.iterate_on = 0;

@@ -175,7 +176,7 @@ TEST_F(sigtrap_threads, enable_event)
 /* Test that modification propagates to all inherited events. */
 TEST_F(sigtrap_threads, modify_and_enable_event)
 {
-       struct perf_event_attr new_attr = make_event_attr(true, &ctx.iterate_on);
+       struct perf_event_attr new_attr = make_event_attr(true, &ctx.iterate_on, 42);
 
        EXPECT_EQ(ioctl(self->fd, PERF_EVENT_IOC_MODIFY_ATTRIBUTES, &new_attr), 0);
        run_test_threads(_metadata, self);

@@ -184,7 +185,7 @@ TEST_F(sigtrap_threads, modify_and_enable_event)
        EXPECT_EQ(ctx.tids_want_signal, 0);
        EXPECT_EQ(ctx.first_siginfo.si_addr, &ctx.iterate_on);
        EXPECT_EQ(ctx.first_siginfo.si_perf_type, PERF_TYPE_BREAKPOINT);
-       EXPECT_EQ(ctx.first_siginfo.si_perf_data, TEST_SIG_DATA(&ctx.iterate_on));
+       EXPECT_EQ(ctx.first_siginfo.si_perf_data, TEST_SIG_DATA(&ctx.iterate_on, 42));
 
        /* Check enabled for parent. */
        ctx.iterate_on = 0;

@@ -204,7 +205,7 @@ TEST_F(sigtrap_threads, signal_stress)
        EXPECT_EQ(ctx.tids_want_signal, 0);
        EXPECT_EQ(ctx.first_siginfo.si_addr, &ctx.iterate_on);
        EXPECT_EQ(ctx.first_siginfo.si_perf_type, PERF_TYPE_BREAKPOINT);
-       EXPECT_EQ(ctx.first_siginfo.si_perf_data, TEST_SIG_DATA(&ctx.iterate_on));
+       EXPECT_EQ(ctx.first_siginfo.si_perf_data, TEST_SIG_DATA(&ctx.iterate_on, 0));
 }
 
 TEST_HARNESS_MAIN
--

2.35.0.rc2.247.g8bbb082509-goog

Due to the alignment requirements of siginfo_t, as described in 3ddb3fd8cdb0 ("signal, perf: Fix siginfo_t by avoiding u64 on 32-bit architectures"), siginfo_t::si_perf_data is limited to an unsigned long.

However, perf_event_attr::sig_data is an u64, to avoid having to deal with compat conversions. Due to being an u64, it may not immediately be clear to users that sig_data is truncated on 32 bit architectures.

Add a comment to explicitly point this out, and hopefully help some users save time by not having to deduce themselves what's happening.

Reported-by Dmitry Vyukov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

---
 include/uapi/linux/perf_event.h | 2 ++
 1 file changed, 2 insertions(+)

include/uapi/linux/perf_event.h +2 -1

index 1b65042ab1db..82858b697c05 100644
@@ -465,6 +465,8 @@ struct perf_event_attr {
        /*
         * User provided data if sigtrap=1, passed back to user via
         * siginfo_t::si_perf_data, e.g. to permit user to identify the event.
+        * Note, siginfo_t::si_perf_data is long-sized, and sig_data will be
+        * truncated accordingly on 32 bit architectures.
         */
        __u64   sig_data;
 };
--

2.35.0.rc2.247.g8bbb082509-goog

The intent has always been that perf_event_attr::sig_data should also be modifiable along with PERF_EVENT_IOC_MODIFY_ATTRIBUTES, because it is observable by user space if SIGTRAP on events is requested.

Currently only PERF_TYPE_BREAKPOINT is modifiable, and explicitly copies relevant breakpoint-related attributes in hw_breakpoint_copy_attr(). This misses copying perf_event_attr::sig_data.

Since sig_data is not specific to PERF_TYPE_BREAKPOINT, introduce a helper to copy generic event-type-independent attributes on modification.

Fixes 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")

Reported-by Dmitry Vyukov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

---
 kernel/events/core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

kernel/events/core.c +16 -1

index fc18664f49b0..db0d85a85f1b 100644
@@ -3197,6 +3197,15 @@ static int perf_event_modify_breakpoint(struct perf_event *bp,
        return err;
 }
 
+/*
+ * Copy event-type-independent attributes that may be modified.
+ */
+static void perf_event_modify_copy_attr(struct perf_event_attr *to,
+                                       const struct perf_event_attr *from)
+{
+       to->sig_data = from->sig_data;
+}
+
 static int perf_event_modify_attr(struct perf_event *event,
                                  struct perf_event_attr *attr)
 {

@@ -3219,10 +3228,17 @@ static int perf_event_modify_attr(struct perf_event *event,
        WARN_ON_ONCE(event->ctx->parent_ctx);
 
        mutex_lock(&event->child_mutex);
+       /*
+        * Event-type-independent attributes must be copied before event-type
+        * modification, which will validate that final attributes match the
+        * source attributes after all relevant attributes have been copied.
+        */
+       perf_event_modify_copy_attr(&event->attr, attr);
        err = func(event, attr);
        if (err)
                goto out;
        list_for_each_entry(child, &event->child_list, child_list) {
+               perf_event_modify_copy_attr(&child->attr, attr);
                err = func(child, attr);
                if (err)
                        goto out;
--

2.35.0.rc2.247.g8bbb082509-goog

Quoted 13 lines · Marco Elver

The intent has always been that perf_event_attr::sig_data should also be modifiable along with PERF_EVENT_IOC_MODIFY_ATTRIBUTES, because it is observable by user space if SIGTRAP on events is requested.

Currently only PERF_TYPE_BREAKPOINT is modifiable, and explicitly copies relevant breakpoint-related attributes in hw_breakpoint_copy_attr(). This misses copying perf_event_attr::sig_data.

Since sig_data is not specific to PERF_TYPE_BREAKPOINT, introduce a helper to copy generic event-type-independent attributes on modification.

Quoted 3 lines

Fixes 97ba62b27867 ("perf: Add support for SIGTRAP on perf events")

Reported-by Dmitry Vyukov <[email protected]>

Signed-off-by Marco Elver <[email protected]>

Reviewed-by Dmitry Vyukov <[email protected]>