2024-02-29 13:05 UTC+0000 ~ Sahil Siddiq <icegambit91@xxxxxxxxx> > 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@xxxxxxxxxxxxx/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@xxxxxxxxx> > --- > 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
