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 <[email protected]>
> > > ---
> > > 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;
On Wed, 24 Mar 2021 at 14:21, Peter Zijlstra <[email protected]> wrote:
>
> 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 <[email protected]>
> > > > ---
> > > > 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.
It'd be nice if this could work. Though I think there's an inherent
problem (same as with fd) with trying to pass a reference back to the
user, while the user can concurrently modify that reference.
Let's assume that user space creates new copies of perf_event_attr for
every version they want, there's still a race where the user modifies
an event, and concurrently in another thread a signal arrives. I
currently don't see a way to determine when it's safe to free a
perf_event_attr or reuse, without there still being a chance that a
signal arrives due to some old perf_event_attr. And for our usecase,
we really need to know a precise subset out of attr that triggered the
event.
So the safest thing I can see is to stash a copy of the relevant
information in siginfo, which is how we ended up with encoding bits
from perf_event_attr into si_perf.
One way around this I could see is that we know that there's a limited
number of combinations of attrs, and the user just creates an instance
for every version they want (and hope it doesn't exceed some large
number). Of course, for breakpoints, we have bp_addr, but let's assume
that si_addr has the right version, so we won't need to access
perf_event_attr::bp_addr.
But given the additional complexities, I'm not sure it's worth it. Is
there a way to solve the modify-signal-race problem in a nicer way?
Thanks,
-- Marco
On Wed, Mar 24, 2021 at 02:21:37PM +0100, Peter Zijlstra wrote:
> --- 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;
Oh bugger; that wants updating for all children too..
> +
> return perf_event_modify_attr(event, &new_attr);
> }
> default:
> @@ -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;
And inherit_event() / perf_event_alloc() want to copy/propagate that.
> if (is_sampling_event(event)) {
> if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
> err = -EOPNOTSUPP;
One last try, I'll leave it alone now, I promise :-)
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -778,6 +778,9 @@ struct perf_event {
void *security;
#endif
struct list_head sb_list;
+
+ unsigned long si_uattr;
+ unsigned long si_data;
#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->si_uattr = (unsigned long)uattr;
+
return perf_event_modify_attr(event, &new_attr);
}
default:
@@ -6399,7 +6403,12 @@ static void perf_sigtrap(struct perf_eve
clear_siginfo(&info);
info.si_signo = SIGTRAP;
info.si_code = TRAP_PERF;
- info.si_errno = event->attr.type;
+ info.si_addr = (void *)event->si_data;
+
+ info.si_perf = event->si_uattr;
+ if (event->parent)
+ info.si_perf = event->parent->si_uattr;
+
force_sig_info(&info);
}
@@ -6414,8 +6423,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 +9130,7 @@ static int __perf_event_overflow(struct
if (events && atomic_dec_and_test(&event->event_limit)) {
ret = 1;
event->pending_kill = POLL_HUP;
+ event->si_data = data->addr;
perf_event_disable_inatomic(event);
}
@@ -12011,6 +12021,8 @@ SYSCALL_DEFINE5(perf_event_open,
goto err_task;
}
+ event->si_uattr = (unsigned long)attr_uptr;
+
if (is_sampling_event(event)) {
if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
err = -EOPNOTSUPP;
On Wed, 24 Mar 2021 at 15:01, Peter Zijlstra <[email protected]> wrote:
>
> One last try, I'll leave it alone now, I promise :-)
This looks like it does what you suggested, thanks! :-)
I'll still need to think about it, because of the potential problem
with modify-signal-races and what the user's synchronization story
would look like then.
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -778,6 +778,9 @@ struct perf_event {
> void *security;
> #endif
> struct list_head sb_list;
> +
> + unsigned long si_uattr;
> + unsigned long si_data;
> #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->si_uattr = (unsigned long)uattr;
> +
> return perf_event_modify_attr(event, &new_attr);
> }
> default:
> @@ -6399,7 +6403,12 @@ static void perf_sigtrap(struct perf_eve
> clear_siginfo(&info);
> info.si_signo = SIGTRAP;
> info.si_code = TRAP_PERF;
> - info.si_errno = event->attr.type;
> + info.si_addr = (void *)event->si_data;
> +
> + info.si_perf = event->si_uattr;
> + if (event->parent)
> + info.si_perf = event->parent->si_uattr;
> +
> force_sig_info(&info);
> }
>
> @@ -6414,8 +6423,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 +9130,7 @@ static int __perf_event_overflow(struct
> if (events && atomic_dec_and_test(&event->event_limit)) {
> ret = 1;
> event->pending_kill = POLL_HUP;
> + event->si_data = data->addr;
>
> perf_event_disable_inatomic(event);
> }
> @@ -12011,6 +12021,8 @@ SYSCALL_DEFINE5(perf_event_open,
> goto err_task;
> }
>
> + event->si_uattr = (unsigned long)attr_uptr;
> +
> if (is_sampling_event(event)) {
> if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
> err = -EOPNOTSUPP;