| Message ID | 20210223143426.2412737-5-elver@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for synchronous signals on perf events | expand |
On Tue, Feb 23, 2021 at 3:34 PM Marco Elver <elver@google.com> 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> > --- > kernel/events/core.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 8718763045fd..d7908322d796 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -6296,6 +6296,17 @@ 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; > + break; > + default: > + /* No additional info set. */ Should we prohibit using attr.sigtrap for !PERF_TYPE_BREAKPOINT if we don't know what info to pass yet? > + break; > + } > + > force_sig_info(&info); > } > > -- > 2.30.0.617.g56c4b15f3c-goog >
On Tue, 23 Feb 2021 at 16:01, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Tue, Feb 23, 2021 at 3:34 PM Marco Elver <elver@google.com> 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> > > --- > > kernel/events/core.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index 8718763045fd..d7908322d796 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -6296,6 +6296,17 @@ 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; > > + break; > > + default: > > + /* No additional info set. */ > > Should we prohibit using attr.sigtrap for !PERF_TYPE_BREAKPOINT if we > don't know what info to pass yet? I don't think it's necessary. This way, by default we get support for other perf events. If user space observes si_perf==0, then there's no information available. That would require that any event type that sets si_perf in future, must ensure that it sets si_perf!=0. I can add a comment to document the requirement here (and user space facing documentation should get a copy of how the info is encoded, too). Alternatively, we could set si_errno to 0 if no info is available, at the cost of losing the type information for events not explicitly listed here. What do you prefer? > > + break; > > + } > > + > > force_sig_info(&info); > > } > > > > -- > > 2.30.0.617.g56c4b15f3c-goog > >
On Tue, Feb 23, 2021 at 4:10 PM 'Marco Elver' via kasan-dev <kasan-dev@googlegroups.com> 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> > > > --- > > > kernel/events/core.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > > index 8718763045fd..d7908322d796 100644 > > > --- a/kernel/events/core.c > > > +++ b/kernel/events/core.c > > > @@ -6296,6 +6296,17 @@ 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; > > > + break; > > > + default: > > > + /* No additional info set. */ > > > > Should we prohibit using attr.sigtrap for !PERF_TYPE_BREAKPOINT if we > > don't know what info to pass yet? > > I don't think it's necessary. This way, by default we get support for > other perf events. If user space observes si_perf==0, then there's no > information available. That would require that any event type that > sets si_perf in future, must ensure that it sets si_perf!=0. > > I can add a comment to document the requirement here (and user space > facing documentation should get a copy of how the info is encoded, > too). > > Alternatively, we could set si_errno to 0 if no info is available, at > the cost of losing the type information for events not explicitly > listed here. > > What do you prefer? Ah, I see. Let's wait for the opinions of other people. There are a number of options for how to approach this. > > > + break; > > > + } > > > + > > > force_sig_info(&info); > > > } > > > > > > -- > > > 2.30.0.617.g56c4b15f3c-goog > > > > > -- > You received this message because you are subscribed to the Google Groups "kasan-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CANpmjNP1wQvG0SNPP2L9QO%3Dnatf0XU8HXj-r2_-U4QZxtr-dVA%40mail.gmail.com.
On Tue, 23 Feb 2021 at 16:16, Dmitry Vyukov <dvyukov@google.com> wrote: > > On Tue, Feb 23, 2021 at 4:10 PM 'Marco Elver' via kasan-dev > <kasan-dev@googlegroups.com> 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> > > > > --- > > > > kernel/events/core.c | 11 +++++++++++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > > > index 8718763045fd..d7908322d796 100644 > > > > --- a/kernel/events/core.c > > > > +++ b/kernel/events/core.c > > > > @@ -6296,6 +6296,17 @@ 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; > > > > + break; > > > > + default: > > > > + /* No additional info set. */ > > > > > > Should we prohibit using attr.sigtrap for !PERF_TYPE_BREAKPOINT if we > > > don't know what info to pass yet? > > > > I don't think it's necessary. This way, by default we get support for > > other perf events. If user space observes si_perf==0, then there's no > > information available. That would require that any event type that > > sets si_perf in future, must ensure that it sets si_perf!=0. > > > > I can add a comment to document the requirement here (and user space > > facing documentation should get a copy of how the info is encoded, > > too). > > > > Alternatively, we could set si_errno to 0 if no info is available, at > > the cost of losing the type information for events not explicitly > > listed here. Note that PERF_TYPE_HARDWARE == 0, so setting si_errno to 0 does not work. Which leaves us with: 1. Ensure si_perf==0 (or some other magic value) if no info is available and !=0 otherwise. 2. Return error for events where we do not officially support requesting sigtrap. I'm currently leaning towards (1). > > What do you prefer? > > Ah, I see. > Let's wait for the opinions of other people. There are a number of > options for how to approach this.
diff --git a/kernel/events/core.c b/kernel/events/core.c index 8718763045fd..d7908322d796 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -6296,6 +6296,17 @@ 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; + break; + default: + /* No additional info set. */ + break; + } + force_sig_info(&info); }
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> --- kernel/events/core.c | 11 +++++++++++ 1 file changed, 11 insertions(+)