On Wed, Mar 24, 2021 at 02:01:56PM +0100, Peter Zijlstra wrote:
On Wed, Mar 24, 2021 at 01:53:48PM +0100, Peter Zijlstra wrote:
On Wed, Mar 24, 2021 at 12:24:59PM +0100, Marco Elver wrote:
Encode information from breakpoint attributes into siginfo_t, which helps disambiguate which breakpoint fired.
Note, providing the event fd may be unreliable, since the event may have been modified (via PERF_EVENT_IOC_MODIFY_ATTRIBUTES) between the event triggering and the signal being delivered to user space.
Signed-off-by: Marco Elver elver@google.com
v2:
- Add comment about si_perf==0.
kernel/events/core.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/kernel/events/core.c b/kernel/events/core.c index 1e4c949bf75f..0316d39e8c8f 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -6399,6 +6399,22 @@ static void perf_sigtrap(struct perf_event *event) info.si_signo = SIGTRAP; info.si_code = TRAP_PERF; info.si_errno = event->attr.type;
- switch (event->attr.type) {
- case PERF_TYPE_BREAKPOINT:
info.si_addr = (void *)(unsigned long)event->attr.bp_addr;info.si_perf = (event->attr.bp_len << 16) | (u64)event->attr.bp_type;Ahh, here's the si_perf user. I wasn't really clear to me what was supposed to be in that field at patch #5 where it was introduced.
Would it perhaps make sense to put the user address of struct perf_event_attr in there instead? (Obviously we'd have to carry it from the syscall to here, but it might be more useful than a random encoding of some bits therefrom).
Then we can also clearly document that's in that field, and it might be more useful for possible other uses.
Something like so...
Ok possibly something like so, which also gets the data address right for more cases.
--- --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -778,6 +778,8 @@ struct perf_event { void *security; #endif struct list_head sb_list; + + struct kernel_siginfo siginfo; #endif /* CONFIG_PERF_EVENTS */ };
--- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5652,13 +5652,17 @@ static long _perf_ioctl(struct perf_even return perf_event_query_prog_array(event, (void __user *)arg);
case PERF_EVENT_IOC_MODIFY_ATTRIBUTES: { + struct perf_event_attr __user *uattr; struct perf_event_attr new_attr; - int err = perf_copy_attr((struct perf_event_attr __user *)arg, - &new_attr); + int err;
+ uattr = (struct perf_event_attr __user *)arg; + err = perf_copy_attr(uattr, &new_attr); if (err) return err;
+ event->siginfo.si_perf = (unsigned long)uattr; + return perf_event_modify_attr(event, &new_attr); } default: @@ -6394,13 +6398,7 @@ void perf_event_wakeup(struct perf_event
static void perf_sigtrap(struct perf_event *event) { - struct kernel_siginfo info; - - clear_siginfo(&info); - info.si_signo = SIGTRAP; - info.si_code = TRAP_PERF; - info.si_errno = event->attr.type; - force_sig_info(&info); + force_sig_info(&event->siginfo); }
static void perf_pending_event_disable(struct perf_event *event) @@ -6414,8 +6412,8 @@ static void perf_pending_event_disable(s WRITE_ONCE(event->pending_disable, -1);
if (event->attr.sigtrap) { - atomic_set(&event->event_limit, 1); /* rearm event */ perf_sigtrap(event); + atomic_set_release(&event->event_limit, 1); /* rearm event */ return; }
@@ -9121,6 +9119,7 @@ static int __perf_event_overflow(struct if (events && atomic_dec_and_test(&event->event_limit)) { ret = 1; event->pending_kill = POLL_HUP; + event->siginfo.si_addr = (void *)data->addr;
perf_event_disable_inatomic(event); } @@ -12011,6 +12010,11 @@ SYSCALL_DEFINE5(perf_event_open, goto err_task; }
+ clear_siginfo(&event->siginfo); + event->siginfo.si_signo = SIGTRAP; + event->siginfo.si_code = TRAP_PERF; + event->siginfo.si_perf = (unsigned long)attr_uptr; + if (is_sampling_event(event)) { if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) { err = -EOPNOTSUPP;