On 01/04/2024 20:04, Sahil Siddiq wrote: > 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") > > Changes since v1: > - Split "mount_bpffs_for_pin" into two functions. > This is done to improve maintainability and readability. > > Changes since v2: > - mount_bpffs_for_pin: rename to "create_and_mount_bpffs_dir". > - mount_bpffs_given_file: rename to "mount_bpffs_given_file". > - create_and_mount_bpffs_dir: > - introduce "dir_exists" boolean. > - remove new dir if "mnt_fs" fails. > - improve error handling and error messages. > > Changes since v3: > - Rectify function name. > - Improve error messages and formatting. > - mount_bpffs_for_file: > - Check if dir exists before block_mount check. > > Signed-off-by: Sahil Siddiq <icegambit91@xxxxxxxxx> Tested-by: Quentin Monnet <qmo@xxxxxxxxxx> Reviewed-by: Quentin Monnet <qmo@xxxxxxxxxx> > --- > tools/bpf/bpftool/common.c | 98 +++++++++++++++++++++++++++++----- > tools/bpf/bpftool/iter.c | 2 +- > tools/bpf/bpftool/main.h | 3 +- > tools/bpf/bpftool/prog.c | 5 +- > tools/bpf/bpftool/struct_ops.c | 2 +- > 5 files changed, 94 insertions(+), 16 deletions(-) > > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c > index cc6e6aae2447..56a5abbb1bf8 100644 > --- a/tools/bpf/bpftool/common.c > +++ b/tools/bpf/bpftool/common.c > @@ -244,29 +244,103 @@ int open_obj_pinned_any(const char *path, enum bpf_obj_type exp_type) > return fd; > } > > -int mount_bpffs_for_pin(const char *name, bool is_dir) > +int create_and_mount_bpffs_dir(const char *dir_name) > { > char err_str[ERR_MAX_LEN]; > - char *file; > - char *dir; > + bool dir_exists; > int err = 0; > > - if (is_dir && is_bpffs(name)) > + if (is_bpffs(dir_name)) > return err; > > - file = malloc(strlen(name) + 1); > - if (!file) { > + dir_exists = (access(dir_name, F_OK) == 0); > + > + if (!dir_exists) { > + char *temp_name; > + char *parent_name; > + > + temp_name = malloc(strlen(dir_name) + 1); > + if (!temp_name) { > + p_err("mem alloc failed"); > + return -1; > + } > + > + strcpy(temp_name, dir_name); > + parent_name = dirname(temp_name); > + > + if (is_bpffs(parent_name)) { > + /* nothing to do if already mounted */ > + free(temp_name); > + return err; > + } > + > + if (access(parent_name, F_OK) == -1) { > + p_err("can't create dir '%s' to pin BPF object: parent dir '%s' doesn't exist", > + dir_name, parent_name); > + free(temp_name); > + return -1; > + } > + > + free(temp_name); > + } > + > + if (block_mount) { > + p_err("no BPF file system found, not mounting it due to --nomount option"); > + return -1; > + } > + > + if (!dir_exists) { > + err = mkdir(dir_name, 0700); > + if (err) { > + p_err("failed to create dir '%s': %s", dir_name, strerror(errno)); > + return err; > + } > + } > + > + err = mnt_fs(dir_name, "bpf", err_str, ERR_MAX_LEN); > + if (err) { > + err_str[ERR_MAX_LEN - 1] = '\0'; > + p_err("can't mount BPF file system on given dir '%s': %s", > + dir_name, err_str); > + > + if (!dir_exists) > + rmdir(dir_name); > + } > + > + return err; > +} > + > +int mount_bpffs_for_file(const char *file_name) > +{ > + char err_str[ERR_MAX_LEN]; > + char *temp_name; > + char *dir; > + int err = 0; > + > + if (access(file_name, F_OK) != -1) { > + p_err("can't pin BPF object: path '%s' already exists", file_name); > + return -1; > + } > + > + temp_name = malloc(strlen(file_name) + 1); > + if (!temp_name) { > p_err("mem alloc failed"); > return -1; > } > > - strcpy(file, name); > - dir = dirname(file); > + strcpy(temp_name, file_name); > + dir = dirname(temp_name); > > if (is_bpffs(dir)) > /* nothing to do if already mounted */ > goto out_free; > > + if (access(dir, F_OK) == -1) { > + p_err("can't pin BPF object: dir '%s' doesn't exist", dir); > + free(temp_name); > + return -1; Or: p_err(...); err = -1; goto out_free; to be more consistent with the other error paths. But that's fine, no need to respin for that. Thanks a lot!
