Re: [PATCH bpf-next v3 1/2] bpftool: Use appropriate permissions for map access

[Quentin Monnet]

2025-06-12 08:18 UTC+1000 ~ Slava Imameev <slava.imameev@xxxxxxxxxxxxxxx>
> Modify several functions in tools/bpf/bpftool/common.c to allow
> specification of requested access for file descriptors, such as
> read-only access.
> 
> Update bpftool to request only read access for maps when write
> access is not required. This fixes errors when reading from maps
> that are protected from modification via security_bpf_map.
> 
> Signed-off-by: Slava Imameev <slava.imameev@xxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - fix for a test compilation error: "conflicting types for 'bpf_fentry_test1'"
> Changes in v3:
> - Addressed review feedback
> - Converted the check for flags to an assert in map_parse_fds
> - Modified map_fd_by_name to keep an existing fd where possible
> - Fixed requested access for map delete command in do_delete
> - Changed requested access to RDONLY for inner map fd in do_create
> - Changed requested access to RDONLY for iterator fd in do_pin
> ---
> ---
>  tools/bpf/bpftool/btf.c           |  3 +-
>  tools/bpf/bpftool/common.c        | 58 ++++++++++++++++++++++---------
>  tools/bpf/bpftool/iter.c          |  2 +-
>  tools/bpf/bpftool/link.c          |  2 +-
>  tools/bpf/bpftool/main.h          | 13 ++++---
>  tools/bpf/bpftool/map.c           | 56 +++++++++++++++++------------
>  tools/bpf/bpftool/map_perf_ring.c |  3 +-
>  tools/bpf/bpftool/prog.c          |  4 +--
>  8 files changed, 91 insertions(+), 50 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6b14cbfa58aa..1ba27cb03348 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -905,7 +905,8 @@ static int do_dump(int argc, char **argv)
>  			return -1;
>  		}
>  
> -		fd = map_parse_fd_and_info(&argc, &argv, &info, &len);
> +		fd = map_parse_fd_and_info(&argc, &argv, &info, &len,
> +					   BPF_F_RDONLY);
>  		if (fd < 0)
>  			return -1;
>  
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index ecfa790adc13..3bdc65112c0d 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c

[...]

> @@ -1023,8 +1042,13 @@ static int map_fd_by_name(char *name, int **fds)
>  	return -1;
>  }
>  
> -int map_parse_fds(int *argc, char ***argv, int **fds)
> +int map_parse_fds(int *argc, char ***argv, int **fds, __u32 open_flags)
>  {
> +	LIBBPF_OPTS(bpf_get_fd_by_id_opts, opts);
> +
> +	assert((open_flags & ~BPF_F_RDONLY) == 0);


Can you please "#include <assert.h>" at the top of the file? We don't
need it in the kernel repo, because the header is included from
tools/include/linux/kernel.h (from bpftool's main.h) if I remember
correctly. But the GitHub mirror uses a stripped-down version of
kernel.h which doesn't pull assert.h, so we need to include it
explicitly - I just remembered when seeing your v3, sorry.

Looks good to me otherwise, thanks!
Quentin