Re: [PATCH v4 bpf-next 1/3] uapi/bpf: fix helper docs

[Daniel Borkmann <daniel@iogearbox.net>]

On Sun, Oct 06, 2019 at 08:07:36PM -0700, Andrii Nakryiko wrote:
> Various small fixes to BPF helper documentation comments, enabling
> automatic header generation with a list of BPF helpers.
> 
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
>  include/uapi/linux/bpf.h       | 32 ++++++++++++++++----------------
>  tools/include/uapi/linux/bpf.h | 32 ++++++++++++++++----------------
>  2 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 77c6be96d676..a65c3b0c6935 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -794,7 +794,7 @@ union bpf_attr {
>   * 		A 64-bit integer containing the current GID and UID, and
>   * 		created as such: *current_gid* **<< 32 \|** *current_uid*.

Overall, I do like the approach that we keep generating the BPF helpers header
file from this documentation as it really enforces that the signatures here
must be 100% correct, and given this also lands in the man page it is /always/
in sync.

> - * int bpf_get_current_comm(char *buf, u32 size_of_buf)
> + * int bpf_get_current_comm(void *buf, u32 size_of_buf)

You did not elaborate why this needs to change from char * to void *. What is
the reason? Those rules should probably be documented somewhere, otherwise
people might keep adding them.

>   * 	Description
>   * 		Copy the **comm** attribute of the current task into *buf* of
>   * 		*size_of_buf*. The **comm** attribute contains the name of
> @@ -1023,7 +1023,7 @@ union bpf_attr {
>   * 		The realm of the route for the packet associated to *skb*, or 0
>   * 		if none was found.
>   *
> - * int bpf_perf_event_output(struct pt_regs *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)
> + * int bpf_perf_event_output(void *ctx, struct bpf_map *map, u64 flags, void *data, u64 size)

This one here is because we have multiple program types with different input context.

>   * 	Description
>   * 		Write raw *data* blob into a special BPF perf event held by
>   * 		*map* of type **BPF_MAP_TYPE_PERF_EVENT_ARRAY**. This perf
> @@ -1068,7 +1068,7 @@ union bpf_attr {
>   * 	Return
>   * 		0 on success, or a negative error in case of failure.
>   *
> - * int bpf_skb_load_bytes(const struct sk_buff *skb, u32 offset, void *to, u32 len)
> + * int bpf_skb_load_bytes(const void *skb, u32 offset, void *to, u32 len)

Changing from struct sk_buff * to void * here, again due to struct sk_reuseport_kern *?

I'm wondering whether it would simply be much better to always just use 'void *ctx'
for everything that is BPF context as it may be just confusing to people why different
types are chosen sometimes leading to buggy drive-by attempts to 'fix' them back into
struct sk_buff * et al.

>   * 	Description
>   * 		This helper was provided as an easy way to load data from a
>   * 		packet. It can be used to load *len* bytes from *offset* from
> @@ -1085,7 +1085,7 @@ union bpf_attr {
>   * 	Return
>   * 		0 on success, or a negative error in case of failure.
>   *
> - * int bpf_get_stackid(struct pt_regs *ctx, struct bpf_map *map, u64 flags)
> + * int bpf_get_stackid(void *ctx, struct bpf_map *map, u64 flags)
>   * 	Description
>   * 		Walk a user or a kernel stack and return its id. To achieve
>   * 		this, the helper needs *ctx*, which is a pointer to the context
> @@ -1154,7 +1154,7 @@ union bpf_attr {
>   * 		The checksum result, or a negative error code in case of
>   * 		failure.
>   *
> - * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, u8 *opt, u32 size)
> + * int bpf_skb_get_tunnel_opt(struct sk_buff *skb, void *opt, u32 size)

Same here and in more places in this patch, why u8 * -> void * and the like?

>   * 	Description
>   * 		Retrieve tunnel options metadata for the packet associated to
>   * 		*skb*, and store the raw tunnel option data to the buffer *opt*
[...]