Re: [PATCH bpf-next v1 6/7] tools: bpftool: fix bpffs mount when pinning subdirectories

[Quentin Monnet <quentin.monnet@netronome.com>]

2019-03-20 18:33 UTC+0100 ~ Alban Crequy <alban.crequy@gmail.com>
> From: Alban Crequy <alban@kinvolk.io>
> 
> When trying to pin at a path such as /sys/fs/bpf/subdir/mymap while the
> bpffs is mounted at the standard path and the "subdir" directory didn't
> exist, bpftool was failing and attempting to mount the bpffs on
> /sys/fs/bpf/subdir/. The mount obviously failed, since "subdir" didn't
> exist.
> 
> Commit 3fc27b71b894 ("tools: bpftool: try to mount bpffs if required for
> pinning objects") says:
> 
>     The code for mnt_bpffs() is a copy of function bpf_mnt_fs() from
>     iproute2 package (under lib/bpf.c, taken at commit 4b73d52f8a81), with
>     modifications regarding handling of error messages.
> 
> However, the function bpf_mnt_fs() from iproute2 only works when its
> parameter is the root of the bpffs (e.g. "/sys/fs/bfs").
For bpftool, the point was to be able to mount the bpffs at any mount
point the user wanted to use. So one way to fix that would be to simply
prevent the mount attempt if the directory does not exist. Another one
would be to attempt to create the directories (but I guess that sounds
more hazardous already). I'm not really sure we should restrict mounting
to the default /sys/fs/bpf. I understand the environment variable still
allows it, so why not, but I find it less straightforward. Maybe others
will have an opinion.

> 
> iproute2/tc actually only mounts at the standard path "/sys/fs/bfs" or
> at the path from the environment variable $TC_BPF_MNT. This patch
> implements a similar strategy but uses the environment variable name
> $BPFTOOL_BPF_MNT.
> 
> This patch still will not do the mkdir automatically. But at least,
> there will be no error message about the mount.
> 
> Fixes: 3fc27b71b894 ("tools: bpftool: try to mount bpffs if required for pinning objects")
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
> ---
>  tools/bpf/bpftool/common.c | 20 ++++++++++++++++----
>  tools/bpf/bpftool/main.h   |  6 ++++++
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index e560cd8f66bc..4783cbbe8037 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -168,6 +168,8 @@ int mount_bpffs_for_pin(const char *name)
>  	char *file;
>  	char *dir;
>  	int err = 0;
> +	const char *mnt_env = getenv(BPF_DIR_MNT_ENV);
> +	static const char *mnt;

Reverse-Christmas tree style for the declarations, please.

>  
>  	file = malloc(strlen(name) + 1);
>  	strcpy(file, name);
> @@ -183,13 +185,23 @@ int mount_bpffs_for_pin(const char *name)
>  		goto out_free;
>  	}
>  
> -	err = mnt_fs(dir, "bpf", err_str, ERR_MAX_LEN);
> +	mnt = mnt_env ? : BPF_DIR_MNT;
> +
> +	// Don't try to mount if wrong prefix: we don't want a leftover mount that is
> +	// not going to help.

Please stick to /* */-style comments.

> +	if (!is_prefix(mnt, dir)) {
> +		p_err("refuse to mount BPF file system (%s) to pin the object (%s): wrong directory",

Not all users know that bpftool attempts to mount the bpffs, I fear this
message might sound a bit obscure. Maybe something along
"<dir> is not in bpffs or under an expected mountpoint, cannot pin ..."?

> +		      mnt, name);
> +		err = -1;
> +		goto out_free;
> +	}
> +
> +	err = mnt_fs(mnt, "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",
> -		      name, err_str);
> +		p_err("can't mount BPF file system (%s) to pin the object (%s): %s",
> +		      mnt, name, err_str);
>  	}
> -

No need to remove the empty line?

>  out_free:
>  	free(file);
>  	return err;
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 7fc2973446d0..a2e28e600b72 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -18,6 +18,12 @@
>  
>  #define ptr_to_u64(ptr)	((__u64)(unsigned long)(ptr))
>  
> +/* If bpffs is not mounted, it can be automatically mounted at this location.
> + * The location can be changed with the environment variable $BPFTOOL_BPF_MNT.

Could you please document that in the manual pages? Otherwise very few
people will ever discover this variable. I don't think we have mentioned
anywhere that we try to mount the file system yet, but we could have it
in both bpftool-prog and bpftool-map I suppose.

> + */
> +#define BPF_DIR_MNT	"/sys/fs/bpf"
> +#define BPF_DIR_MNT_ENV	"BPFTOOL_BPF_MNT"

Could we rename those macros into BPFFS_MNT_* maybe?

> +
>  #define NEXT_ARG()	({ argc--; argv++; if (argc < 0) usage(); })
>  #define NEXT_ARGP()	({ (*argc)--; (*argv)++; if (*argc < 0) usage(); })
>  #define BAD_ARG()	({ p_err("what is '%s'?", *argv); -1; })
>