| Message ID | 20200626001332.1554603-2-songliubraving@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: introduce bpf_get_task_stack() | expand |
On Thu, Jun 25, 2020 at 05:13:29PM -0700, Song Liu wrote:
> This would be used by bpf stack mapo.
Would it make sense to sanitize the API a little before exposing it?
diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index 334d48b16c36..016894b0d2c2 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -159,8 +159,10 @@ static struct perf_callchain_entry *get_callchain_entry(int *rctx)
return NULL;
entries = rcu_dereference(callchain_cpus_entries);
- if (!entries)
+ if (!entries) {
+ put_recursion_context(this_cpu_ptr(callchain_recursion), rctx);
return NULL;
+ }
cpu = smp_processor_id();
@@ -183,12 +185,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user,
int rctx;
entry = get_callchain_entry(&rctx);
- if (rctx == -1)
+ if (!entry || rctx == -1)
return NULL;
- if (!entry)
- goto exit_put;
-
ctx.entry = entry;
ctx.max_stack = max_stack;
ctx.nr = entry->nr = init_nr;
On Fri, Jun 26, 2020 at 5:10 AM Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Jun 25, 2020 at 05:13:29PM -0700, Song Liu wrote: > > This would be used by bpf stack mapo. > > Would it make sense to sanitize the API a little before exposing it? > > diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c > index 334d48b16c36..016894b0d2c2 100644 > --- a/kernel/events/callchain.c > +++ b/kernel/events/callchain.c > @@ -159,8 +159,10 @@ static struct perf_callchain_entry *get_callchain_entry(int *rctx) > return NULL; > > entries = rcu_dereference(callchain_cpus_entries); > - if (!entries) > + if (!entries) { > + put_recursion_context(this_cpu_ptr(callchain_recursion), rctx); > return NULL; > + } > > cpu = smp_processor_id(); > > @@ -183,12 +185,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > int rctx; > > entry = get_callchain_entry(&rctx); > - if (rctx == -1) > + if (!entry || rctx == -1) > return NULL; > isn't rctx == -1 check here not necessary anymore? Seems like get_callchain_entry() will always return NULL if rctx == -1? > - if (!entry) > - goto exit_put; > - > ctx.entry = entry; > ctx.max_stack = max_stack; > ctx.nr = entry->nr = init_nr;
> On Jun 26, 2020, at 4:00 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Jun 25, 2020 at 05:13:29PM -0700, Song Liu wrote: >> This would be used by bpf stack mapo. > > Would it make sense to sanitize the API a little before exposing it? I will fold this in the next version. Thanks, Song > > diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c > index 334d48b16c36..016894b0d2c2 100644 > --- a/kernel/events/callchain.c > +++ b/kernel/events/callchain.c > @@ -159,8 +159,10 @@ static struct perf_callchain_entry *get_callchain_entry(int *rctx) > return NULL; > > entries = rcu_dereference(callchain_cpus_entries); > - if (!entries) > + if (!entries) { > + put_recursion_context(this_cpu_ptr(callchain_recursion), rctx); > return NULL; > + } > > cpu = smp_processor_id(); > > @@ -183,12 +185,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, > int rctx; > > entry = get_callchain_entry(&rctx); > - if (rctx == -1) > + if (!entry || rctx == -1) > return NULL; > > - if (!entry) > - goto exit_put; > - > ctx.entry = entry; > ctx.max_stack = max_stack; > ctx.nr = entry->nr = init_nr;
> On Jun 26, 2020, at 1:06 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Jun 26, 2020 at 5:10 AM Peter Zijlstra <peterz@infradead.org> wrote: >> >> On Thu, Jun 25, 2020 at 05:13:29PM -0700, Song Liu wrote: >>> This would be used by bpf stack mapo. >> >> Would it make sense to sanitize the API a little before exposing it? >> >> diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c >> index 334d48b16c36..016894b0d2c2 100644 >> --- a/kernel/events/callchain.c >> +++ b/kernel/events/callchain.c >> @@ -159,8 +159,10 @@ static struct perf_callchain_entry *get_callchain_entry(int *rctx) >> return NULL; >> >> entries = rcu_dereference(callchain_cpus_entries); >> - if (!entries) >> + if (!entries) { >> + put_recursion_context(this_cpu_ptr(callchain_recursion), rctx); >> return NULL; >> + } >> >> cpu = smp_processor_id(); >> >> @@ -183,12 +185,9 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, >> int rctx; >> >> entry = get_callchain_entry(&rctx); >> - if (rctx == -1) >> + if (!entry || rctx == -1) >> return NULL; >> > > isn't rctx == -1 check here not necessary anymore? Seems like > get_callchain_entry() will always return NULL if rctx == -1? Yes, looks like we only need to check entry. Thanks, Song
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index b4bb32082342c..00ab5efa38334 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1244,6 +1244,8 @@ get_perf_callchain(struct pt_regs *regs, u32 init_nr, bool kernel, bool user, extern struct perf_callchain_entry *perf_callchain(struct perf_event *event, struct pt_regs *regs); extern int get_callchain_buffers(int max_stack); extern void put_callchain_buffers(void); +extern struct perf_callchain_entry *get_callchain_entry(int *rctx); +extern void put_callchain_entry(int rctx); extern int sysctl_perf_event_max_stack; extern int sysctl_perf_event_max_contexts_per_stack; diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c index 334d48b16c36d..50b8a1622807f 100644 --- a/kernel/events/callchain.c +++ b/kernel/events/callchain.c @@ -149,7 +149,7 @@ void put_callchain_buffers(void) } } -static struct perf_callchain_entry *get_callchain_entry(int *rctx) +struct perf_callchain_entry *get_callchain_entry(int *rctx) { int cpu; struct callchain_cpus_entries *entries; @@ -168,7 +168,7 @@ static struct perf_callchain_entry *get_callchain_entry(int *rctx) (*rctx * perf_callchain_entry__sizeof())); } -static void +void put_callchain_entry(int rctx) { put_recursion_context(this_cpu_ptr(callchain_recursion), rctx);
This would be used by bpf stack mapo. Signed-off-by: Song Liu <songliubraving@fb.com> --- include/linux/perf_event.h | 2 ++ kernel/events/callchain.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-)