2023-05-04 18:45 UTC+0800 ~ Pengcheng Yang <yangpc@xxxxxxxxxx>
> Currently, when using prog loadall, if the pin path is a bpffs
> mountpoint, bpffs will be repeatedly mounted to the parent directory
> of the bpffs mountpoint path.
>
> For example,
> $ bpftool prog loadall test.o /sys/fs/bpf
> currently bpffs will be repeatedly mounted to /sys/fs.
>
> Signed-off-by: Pengcheng Yang <yangpc@xxxxxxxxxx>
> ---
> tools/bpf/bpftool/common.c | 9 ++++++---
> tools/bpf/bpftool/iter.c | 2 +-
> tools/bpf/bpftool/main.h | 2 +-
> tools/bpf/bpftool/prog.c | 2 +-
> 4 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 5a73ccf14332..880fcb45f89f 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -68,7 +68,7 @@ void p_info(const char *fmt, ...)
> va_end(ap);
> }
>
> -static bool is_bpffs(char *path)
> +static bool is_bpffs(const char *path)
> {
> struct statfs st_fs;
>
> @@ -244,13 +244,16 @@ int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type)
> return fd;
> }
>
> -int mount_bpffs_for_pin(const char *name)
> +int mount_bpffs_for_pin(const char *name, bool is_dir)
> {
> char err_str[ERR_MAX_LEN];
> char *file;
> char *dir;
> int err = 0;
>
> + if (is_dir && is_bpffs(name))
> + return err;
> +
> file = malloc(strlen(name) + 1);
> if (!file) {
> p_err("mem alloc failed");
> @@ -286,7 +289,7 @@ int do_pin_fd(int fd, const char *name)
> {
> int err;
>
> - err = mount_bpffs_for_pin(name);
> + err = mount_bpffs_for_pin(name, false);
> if (err)
> return err;
>
> diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c
> index 9a1d2365a297..6b0e5202ca7a 100644
> --- a/tools/bpf/bpftool/iter.c
> +++ b/tools/bpf/bpftool/iter.c
> @@ -76,7 +76,7 @@ static int do_pin(int argc, char **argv)
> goto close_obj;
> }
>
> - err = mount_bpffs_for_pin(path);
> + err = mount_bpffs_for_pin(path, false);
> if (err)
> goto close_link;
>
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0ef373cef4c7..665f23f68066 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -142,7 +142,7 @@ const char *get_fd_type_name(enum bpf_obj_type type);
> char *get_fdinfo(int fd, const char *key);
> int open_obj_pinned(const char *path, bool quiet);
> int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type);
> -int mount_bpffs_for_pin(const char *name);
> +int mount_bpffs_for_pin(const char *name, bool is_dir);
> int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(int *, char ***));
> int do_pin_fd(int fd, const char *name);
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index afbe3ec342c8..473ec01c00d6 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1747,7 +1747,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> goto err_close_obj;
> }
>
> - err = mount_bpffs_for_pin(pinfile);
> + err = mount_bpffs_for_pin(pinfile, !first_prog_only);
> if (err)
> goto err_close_obj;
>
Thanks! Makes sense to pass down is_dir, given that the directory does
not always exist at this stage so we can't just fstat(name) to check the
type in mount_bpffs_for_pin().
Note that you missed an occurrence of mount_bpffs_for_pin() in
struct_ops.c, recently added in commit 0232b7889786 ("bpftool: Register
struct_ops with a link."), please fix it.
I realise that even if we pass a directory name, we try to mount the
bpffs on the parent directory. We should clean this up in the future and
only mount on the provided directory in this case, but this might
require creating the directory first (it does not always exist at this
stage).
Thanks,
Quentin
