Re: [PATCH v2 bpf-next 1/3] bpf: Add bpf_perf_prog_read_branches() helper

[Daniel Borkmann <daniel@iogearbox.net>]

On 1/23/20 9:09 PM, Daniel Xu wrote:
> Hi John, thanks for looking.
> 
> On Wed Jan 22, 2020 at 9:39 PM, John Fastabend wrote:
> [...]
>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>> index 033d90a2282d..7350c5be6158 100644
>>> --- a/include/uapi/linux/bpf.h
>>> +++ b/include/uapi/linux/bpf.h
>>> @@ -2885,6 +2885,16 @@ union bpf_attr {
>>>    *		**-EPERM** if no permission to send the *sig*.
>>>    *
>>>    *		**-EAGAIN** if bpf program can try again.
>>> + *
>>> + * int bpf_perf_prog_read_branches(struct bpf_perf_event_data *ctx, void *buf, u32 buf_size)
>>> + * 	Description
>>> + * 		For en eBPF program attached to a perf event, retrieve the
>>> + * 		branch records (struct perf_branch_entry) associated to *ctx*
>>> + * 		and store it in	the buffer pointed by *buf* up to size
>>> + * 		*buf_size* bytes.
>>
>> It seems extra bytes in buf will be cleared. The number of bytes
>> copied is returned so I don't see any reason to clear the extra bytes I
>> would
>> just let the BPF program do this if they care. But it should be noted in
>> the description at least.
> 
> In include/linux/bpf.h:
> 
>          /* the following constraints used to prototype bpf_memcmp() and other
>           * functions that access data on eBPF program stack
>           */
>          ARG_PTR_TO_UNINIT_MEM,  /* pointer to memory does not need to be initialized,
>                                   * helper function must fill all bytes or clear
>                                   * them in error case.
>                                   */
> 
> I figured it would be good to clear out the stack b/c this helper
> writes data on program stack.
> 
> Also bpf_perf_prog_read_value() does something similar (fill zeros on
> failure).
> 
> [...]
>>> +	to_copy = min_t(u32, br_stack->nr * sizeof(struct perf_branch_entry), size);
>>> +	to_clear -= to_copy;
>>> +
>>> +	memcpy(buf, br_stack->entries, to_copy);
>>> +	err = to_copy;
>>> +clear:
>>> +	memset(buf + to_copy, 0, to_clear);
>>
>>
>> Here, why do this at all? If the user cares they can clear the bytes
>> directly from the BPF program. I suspect its probably going to be
>> wasted work in most cases. If its needed for some reason provide
>> a comment with it.
> 
> Same concern as above, right?

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.

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?

Thanks,
Daniel