Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Quentin Monnet <quentin@isovalent.com>]

2024-02-29 13:05 UTC+0000 ~ Sahil Siddiq <icegambit91@gmail.com>
> When pinning programs/objects under PATH (eg: during "bpftool prog
> loadall") the bpffs is mounted on the parent dir of PATH in the
> following situations:
> - the given dir exists but it is not bpffs.
> - the given dir doesn't exist and the parent dir is not bpffs.
> 
> Mounting on the parent dir can also have the unintentional side-
> effect of hiding other files located under the parent dir.
> 
> If the given dir exists but is not bpffs, then the bpffs should
> be mounted on the given dir and not its parent dir.
> 
> Similarly, if the given dir doesn't exist and its parent dir is not
> bpffs, then the given dir should be created and the bpffs should be
> mounted on this new dir.
> 
> Link: https://lore.kernel.org/bpf/2da44d24-74ae-a564-1764-afccf395eeec@isovalent.com/T/#t
> 
> Closes: https://github.com/libbpf/bpftool/issues/100
> 

Fixes: 2a36c26fe3b8 ("bpftool: Support bpffs mountpoint as pin path for prog loadall")

> Signed-off-by: Sahil Siddiq <icegambit91@gmail.com>
> ---
>  tools/bpf/bpftool/common.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index cc6e6aae2447..6b2c3e82c19e 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -254,6 +254,17 @@ int mount_bpffs_for_pin(const char *name, bool is_dir)
>  	if (is_dir && is_bpffs(name))
>  		return err;
>  
> +	if (is_dir && access(name, F_OK) != -1) {
> +		err = mnt_fs(name, "bpf", err_str, ERR_MAX_LEN);
> +		if (err) {
> +			err_str[ERR_MAX_LEN - 1] = '\0';
> +			p_err("can't mount BPF file system to pin the object (%s): %s",

The error string should be updated, we're not trying to pin one object
file here but to mount the bpffs on a directory to pin several objects.

> +				name, err_str);

Formatting nit: "name" should be aligned with the argument from the line
above (the opening double quote). You can catch this by running
"./scripts/checkpatch.pl --strict" on your patch/commit.

> +		}
> +
> +		return err;
> +	}

This block above cannot be before the check on "block_mount", or we will
ignore the "--nomount" option if the user passes it.

Perhaps it would be clearer to split the logics of mount_bpffs_for_pin()
into two subfunctions, one for directories, one for file paths. This way
we would avoid to call malloc() and dirname() when "name" is already a
directory, and it would be easier to follow the different cases.

> +
>  	file = malloc(strlen(name) + 1);
>  	if (!file) {
>  		p_err("mem alloc failed");
> @@ -273,7 +284,17 @@ int mount_bpffs_for_pin(const char *name, bool is_dir)
>  		goto out_free;
>  	}
>  
> -	err = mnt_fs(dir, "bpf", err_str, ERR_MAX_LEN);
> +	if (is_dir) {
> +		err = mkdir(name, 0700);
> +		if (err) {
> +			err_str[ERR_MAX_LEN - 1] = '\0';
> +			p_err("failed to mkdir (%s): %s",
> +				name, err_str);

We use err_str to pass it to mnt_fs, but we cannot use it here (it is
not set by mkdir). We probably want "strerror(errno)" instead.

+ Formatting nit: "name" should be aligned with the opening quote (use
spaces for the last part of the indentation).

> +			goto out_free;
> +		}
> +	}
> +
> +	err = mnt_fs(is_dir ? name : dir, "bpf", err_str, ERR_MAX_LEN);
>  	if (err) {
>  		err_str[ERR_MAX_LEN - 1] = '\0';
>  		p_err("can't mount BPF file system to pin the object (%s): %s",

Thanks for this work!
Quentin