On Wed, Mar 24, 2021 at 3:05 PM Marco Elver <[email protected]> wrote:
>
> 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.
I agree that this looks inherently racy. The attr can't be allocated
on stack, user synchronization may be tricky and expensive. The API
may provoke bugs and some users may not even realize the race problem.
One potential alternative is use of an opaque u64 context (if we could
shove it into the attr). A user can pass a pointer to the attr in
there (makes it equivalent to this proposal), or bit-pack size/type
(as we want), pass some sequence number or whatever.
> > --- 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, Mar 24, 2021 at 3:12 PM Dmitry Vyukov <[email protected]> wrote:
> > 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.
>
> I agree that this looks inherently racy. The attr can't be allocated
> on stack, user synchronization may be tricky and expensive. The API
> may provoke bugs and some users may not even realize the race problem.
>
> One potential alternative is use of an opaque u64 context (if we could
> shove it into the attr). A user can pass a pointer to the attr in
> there (makes it equivalent to this proposal), or bit-pack size/type
> (as we want), pass some sequence number or whatever.
Just to clarify what I was thinking about, but did not really state:
perf_event_attr_t includes u64 ctx, and we return it back to the user
in siginfo_t. Kernel does not treat it in any way. This is a pretty
common API pattern in general.
> > > --- 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:15, Dmitry Vyukov <[email protected]> wrote:
> On Wed, Mar 24, 2021 at 3:12 PM Dmitry Vyukov <[email protected]> wrote:
> > > 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.
> >
> > I agree that this looks inherently racy. The attr can't be allocated
> > on stack, user synchronization may be tricky and expensive. The API
> > may provoke bugs and some users may not even realize the race problem.
> >
> > One potential alternative is use of an opaque u64 context (if we could
> > shove it into the attr). A user can pass a pointer to the attr in
> > there (makes it equivalent to this proposal), or bit-pack size/type
> > (as we want), pass some sequence number or whatever.
>
> Just to clarify what I was thinking about, but did not really state:
> perf_event_attr_t includes u64 ctx, and we return it back to the user
> in siginfo_t. Kernel does not treat it in any way. This is a pretty
> common API pattern in general.
Ok, let's go for a new field in perf_event_attr which is copied to
si_perf. This gives user space full flexibility to decide what to
stick in it, and the kernel does not prescribe some weird encoding or
synchronization that user space would have to live with. I'll probably
call it perf_event_attr::sig_data, because all si_* things are macros.
Thanks,
-- Marco
* Dmitry Vyukov <[email protected]> wrote:
> On Wed, Mar 24, 2021 at 3:05 PM Marco Elver <[email protected]> wrote:
> >
> > 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.
>
> I agree that this looks inherently racy. The attr can't be allocated
> on stack, user synchronization may be tricky and expensive. The API
> may provoke bugs and some users may not even realize the race problem.
Yeah, so why cannot we allocate enough space from the signal handler
user-space stack and put the attr there, and point to it from
sig_info?
The idea would be to create a stable, per-signal snapshot of whatever
the perf_attr state is at the moment the event happens and the signal
is generated - which is roughly what user-space wants, right?
Thanks,
Ingo
On Thu, 25 Mar 2021 at 15:18, Ingo Molnar <[email protected]> wrote:
>
> * Dmitry Vyukov <[email protected]> wrote:
>
> > On Wed, Mar 24, 2021 at 3:05 PM Marco Elver <[email protected]> wrote:
> > >
> > > 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.
> >
> > I agree that this looks inherently racy. The attr can't be allocated
> > on stack, user synchronization may be tricky and expensive. The API
> > may provoke bugs and some users may not even realize the race problem.
>
> Yeah, so why cannot we allocate enough space from the signal handler
> user-space stack and put the attr there, and point to it from
> sig_info?
>
> The idea would be to create a stable, per-signal snapshot of whatever
> the perf_attr state is at the moment the event happens and the signal
> is generated - which is roughly what user-space wants, right?
I certainly couldn't say how feasible this is. Is there infrastructure
in place to do this? Or do we have to introduce support for stashing
things on the signal stack?
From what we can tell, the most flexible option though appears to be
just some user settable opaque data in perf_event_attr, that is copied
to siginfo. It'd allow user space to store a pointer or a hash/key, or
just encode the relevant information it wants; but could also go
further, and add information beyond perf_event_attr, such as things
like a signal receiver filter (e.g. task ID or set of threads which
should process the signal etc.).
So if there's no strong objection to the additional field in
perf_event_attr, I think it'll give us the simplest and most flexible
option.
Thanks,
-- Marco
> Thanks,
>
> Ingo
* Marco Elver <[email protected]> wrote:
> > Yeah, so why cannot we allocate enough space from the signal
> > handler user-space stack and put the attr there, and point to it
> > from sig_info?
> >
> > The idea would be to create a stable, per-signal snapshot of
> > whatever the perf_attr state is at the moment the event happens
> > and the signal is generated - which is roughly what user-space
> > wants, right?
>
> I certainly couldn't say how feasible this is. Is there
> infrastructure in place to do this? Or do we have to introduce
> support for stashing things on the signal stack?
>
> From what we can tell, the most flexible option though appears to be
> just some user settable opaque data in perf_event_attr, that is
> copied to siginfo. It'd allow user space to store a pointer or a
> hash/key, or just encode the relevant information it wants; but
> could also go further, and add information beyond perf_event_attr,
> such as things like a signal receiver filter (e.g. task ID or set of
> threads which should process the signal etc.).
>
> So if there's no strong objection to the additional field in
> perf_event_attr, I think it'll give us the simplest and most
> flexible option.
Sounds good to me - it's also probably measurably faster than copying
the not-so-small-anymore perf_attr structure.
Thanks,
Ingo