Messages in this thread | | | Subject | Re: [PATCH v2 bpf-next 1/3] bpf: Add bpf_perf_prog_read_branches() helper | From | Daniel Borkmann <> | Date | Thu, 23 Jan 2020 23:44:53 +0100 |
| |
On 1/23/20 11:30 PM, Daniel Xu wrote: > On Thu Jan 23, 2020 at 11:23 PM, Daniel Borkmann wrote: > [...] >> >> Yes, so we've been following this practice for all the BPF helpers no >> matter >> which program type. Though for tracing it may be up to debate whether it >> makes >> still sense given there's nothing to be leaked here since you can read >> this data >> anyway via probe read if you'd wanted to. So we might as well get rid of >> the >> clearing for all tracing helpers. > > Right, that makes sense. Do you want me to leave it in for this patchset > and then remove all of them in a followup patchset?
Lets leave it in and in a different set, we can clean this up for all tracing related helpers at once.
>> Different question related to your set. It looks like br_stack is only >> available >> on x86, is that correct? For other archs this will always bail out on >> !br_stack >> test. Perhaps we should document this fact so users are not surprised >> why their >> prog using this helper is not working on !x86. Wdyt? > > I think perf_event_open() should fail on !x86 if a user tries to configure > it with branch stack collection. So there would not be the opportunity for > the bpf prog to be attached and run. I haven't tested this, though. I'll > look through the code / install a VM and test it.
As far as I can see the prog would still be attachable and runnable, just that the helper always will return -EINVAL on these archs. Maybe error code should be changed into -ENOENT to avoid confusion wrt whether user provided some invalid input args. Should this actually bail out with -EINVAL if size is not a multiple of sizeof(struct perf_branch_entry) as otherwise we'd end up copying half broken branch entry information?
Thanks, Daniel
| |