| Message ID | 20240308130619.28123-1-icegambit91@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v2] bpftool: Mount bpffs on provided dir instead of parent dir | expand |
Thanks! Apologies for the delay. 2024-03-08 13:07 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> Note: you don't need the blank lines between the tags. > --- > Changes since v1: > - Split "mount_bpffs_for_pin" into two functions: > - mount_bpffs_on_dir > - mount_bpffs_given_file > This is done to improve maintainability and readability. > - Improve error messages > - (mount_bpffs_given_file): If the file already exists, throw an > error instead of waiting for bpf_obj_pin() to throw an error > - Code style changes You can keep the changelog as part of the patch description. > > tools/bpf/bpftool/common.c | 78 +++++++++++++++++++++++++++++----- > 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, 75 insertions(+), 15 deletions(-) > > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c > index cc6e6aae2447..d2746681200e 100644 > --- a/tools/bpf/bpftool/common.c > +++ b/tools/bpf/bpftool/common.c > @@ -244,24 +244,80 @@ 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 mount_bpffs_on_dir(const char *dir_name) With all the checks and the potential directory creation, we could maybe rename this into "prepare_bpffs_dir()" or something like this? > { > char err_str[ERR_MAX_LEN]; > - char *file; > - char *dir; > int err = 0; > > - if (is_dir && is_bpffs(name)) > + if (is_bpffs(dir_name)) > return err; > > - file = malloc(strlen(name) + 1); > - if (!file) { > + if (access(dir_name, F_OK) == -1) { > + 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; > + } > + > + free(temp_name); > + > + if (block_mount) { > + p_err("no BPF file system found, not mounting it due to --nomount option"); > + return -1; > + } > + > + err = mkdir(dir_name, 0700); > + if (err) { > + p_err("failed to create dir (%s): %s", dir_name, strerror(errno)); > + return err; > + } > + } else if (block_mount) { > + p_err("no BPF file system found, not mounting it due to --nomount option"); > + return -1; > + } I'd maybe change this block a little (although it's up to you): bool dir_exists; dire_exists = (access(...) == 0); if (!dir_exists) { ... free(temp_name); } if (block_mount) { ... } if (!dir_exists) { err = mkdir(...); ... } err = mnt_fs(...); ... This would also enable us to remove the directory we just created, if we're not able to mount the bpffs on it, before leaving the function. > + > + 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); > + } > + > + return err; > +} > + > +int mount_bpffs_given_file(const char *file_name) I'd remove/replace "given" from the name, maybe "mount_bpffs_for_file"? > +{ > + char err_str[ERR_MAX_LEN]; > + char *temp_name; > + char *dir; > + int err = 0; > + > + if (access(file_name, F_OK) != -1) { > + p_err("bpf object can't be pinned since file (%s) already exists", file_name); I'd remove "file", the existing object might be a directory and it might be confusing. > + 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); > Here, we could check for the existence of "dir", and error out otherwise. The reason is that with the current code, if dir does not exist but user passes --nomount, then we fail to pin the path (given that the directory is not present), but the message returned will be "no BPF file system found, not mounting it due to --nomount option", which is confusing. Same note applies to the other function as well. > if (is_bpffs(dir)) > /* nothing to do if already mounted */ > @@ -277,11 +333,11 @@ int mount_bpffs_for_pin(const char *name, bool is_dir) > if (err) { > err_str[ERR_MAX_LEN - 1] = '\0'; > p_err("can't mount BPF file system to pin the object (%s): %s", We could also indicate the path where we tried to mount the bpffs, in this message. > - name, err_str); > + file_name, err_str); > } > > out_free: > - free(file); > + free(temp_name); > return err; > } > > @@ -289,7 +345,7 @@ int do_pin_fd(int fd, const char *name) > { > int err; > > - err = mount_bpffs_for_pin(name, false); > + err = mount_bpffs_given_file(name); > if (err) > return err; > > diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c > index 6b0e5202ca7a..3911563dcc60 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, false); > + err = mount_bpffs_given_file(path); > if (err) > goto close_link; > > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h > index b8bb08d10dec..20e06ad183ec 100644 > --- a/tools/bpf/bpftool/main.h > +++ b/tools/bpf/bpftool/main.h > @@ -142,7 +142,8 @@ 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, bool is_dir); > +int mount_bpffs_given_file(const char *file_name); > +int mount_bpffs_on_dir(const char *dir_name); > 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 9cb42a3366c0..09b5f0415a5e 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -1778,7 +1778,10 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) > goto err_close_obj; > } > > - err = mount_bpffs_for_pin(pinfile, !first_prog_only); > + if (first_prog_only) > + err = mount_bpffs_given_file(pinfile); > + else > + err = mount_bpffs_on_dir(pinfile); > if (err) > goto err_close_obj; > > diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c > index d573f2640d8e..bf50a99b2501 100644 > --- a/tools/bpf/bpftool/struct_ops.c > +++ b/tools/bpf/bpftool/struct_ops.c > @@ -515,7 +515,7 @@ static int do_register(int argc, char **argv) > if (argc == 1) > linkdir = GET_ARG(); > > - if (linkdir && mount_bpffs_for_pin(linkdir, true)) { > + if (linkdir && mount_bpffs_on_dir(linkdir)) { > p_err("can't mount bpffs for pinning"); > return -1; > } Other than the comments above, the patch works well, and the different cases are much easier to follow than in v1, thanks! I've checked that the programs load as expected, the directories are created (or not) as expected, and the bpffs is mounted (or not) as expected, for all the cases I could think of, with the following commands (copied here, for the record), all worked as I expected: mkdir -p /sys/fs/bpf/some_dir mkdir -p /tmp/test-dir/some_dir # /tmp/ret.o contains 1 program, /tmp/rets.o contains several ones # Load one program under bpffs bpftool prog load /tmp/ret.o /sys/fs/bpf/foo type xdp mount | grep bpf ls /sys/fs/bpf/foo # Try to load one program when pin path exists, under bpffs bpftool prog load /tmp/ret.o /sys/fs/bpf/foo type xdp mount | grep bpf ls /sys/fs/bpf/foo rm /sys/fs/bpf/foo # Load one program under bpffs, with --nomount bpftool --nomount prog load /tmp/ret.o /sys/fs/bpf/foo type xdp mount | grep bpf ls /sys/fs/bpf/foo rm /sys/fs/bpf/foo # Load one program outside of bpffs, with --nomount bpftool --nomount prog load /tmp/ret.o /tmp/test-dir/foo type xdp mount | grep bpf ls /tmp/test-dir/foo # Load one program outside of bpffs bpftool prog load /tmp/ret.o /tmp/test-dir/foo type xdp mount | grep bpf ls /tmp/test-dir/foo # Try to load one program when pin path exists, outside of bpffs bpftool prog load /tmp/ret.o /tmp/test-dir/foo type xdp mount | grep bpf rm /tmp/test-dir/foo umount /tmp/test-dir umount /tmp/test-dir # Load multiple programs under bpffs, directory exists bpftool prog loadall /tmp/rets.o /sys/fs/bpf/some_dir mount | grep bpf ls /sys/fs/bpf/some_dir rm /sys/fs/bpf/some_dir/* # Load multiple programs under bpffs, with -n, directory exists bpftool --nomount prog loadall /tmp/rets.o /sys/fs/bpf/some_dir mount | grep bpf ls /sys/fs/bpf/some_dir rm /sys/fs/bpf/some_dir/* # Load multiple programs under bpffs, directory does not exist bpftool prog loadall /tmp/rets.o /sys/fs/bpf/new_dir mount | grep bpf ls /sys/fs/bpf/new_dir rm -r /sys/fs/bpf/new_dir # Load multiple progs under bpffs, with -n, directory doesn't exist bpftool --nomount prog loadall /tmp/rets.o /sys/fs/bpf/new_dir mount | grep bpf ls /sys/fs/bpf/new_dir rm -r /sys/fs/bpf/new_dir # Load multiple programs outside of bpffs, directory exists bpftool prog loadall /tmp/rets.o /tmp/test-dir/some_dir mount | grep bpf ls /tmp/test-dir/some_dir rm /tmp/test-dir/some_dir/* umount /tmp/test-dir/some_dir umount /tmp/test-dir/some_dir # Try to load multiple progs outside of bpffs, with -n, dir exists bpftool --nomount prog loadall /tmp/rets.o /tmp/test-dir/some_dir mount | grep bpf ls /tmp/test-dir/some_dir # Load multiple programs outside of bpffs, directory does not exist bpftool prog loadall /tmp/rets.o /tmp/test-dir/new_dir mount | grep bpf ls /tmp/test-dir/new_dir umount /tmp/test-dir/new_dir umount /tmp/test-dir/new_dir rm -r /tmp/test-dir/new_dir # Try to load multiple progs outside of bpffs, with -n, new dir bpftool --nomount prog loadall /tmp/rets.o /tmp/test-dir/new_dir mount | grep bpf ls /tmp/test-dir/new_dir rm -r /tmp/test-dir Thanks, Quentin
Hi, On Wednesday, March 13, 2024 9:17:44 PM IST Quentin Monnet wrote: > Thanks! Apologies for the delay. No worries! Thank you for the review. > [...] > Note: you don't need the blank lines between the tags. > [...] > You can keep the changelog as part of the patch description. Got it. I'll keep this in mind when I submit v3. > [...] > With all the checks and the potential directory creation, we could maybe > rename this into "prepare_bpffs_dir()" or something like this? "prepare_bpffs_dir" is quite apt. If longer names are acceptable then I would also recommend "prepare_and_mount_bpffs_dir" so it indicates that it'll also mount the bpffs on the dir (when relevant) after performing the checks. > [...] > I'd maybe change this block a little (although it's up to you): > > bool dir_exists; > > dire_exists = (access(...) == 0); > if (!dir_exists) { > ... > free(temp_name); > } > > if (block_mount) { > ... > } > > if (!dir_exists) { > err = mkdir(...); > ... > } > > err = mnt_fs(...); > ... > > This would also enable us to remove the directory we just created, if > we're not able to mount the bpffs on it, before leaving the function. I agree with this. This will also keep the implementation a little more succinct with just one "block_mount" conditional block. > [...] > I'd remove/replace "given" from the name, maybe "mount_bpffs_for_file"? This is definitely a better name. I am not very good when it comes to making up names :P > [...] > I'd remove "file", the existing object might be a directory and it might > be confusing. > > > + 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); > > Here, we could check for the existence of "dir", and error out > otherwise. The reason is that with the current code, if dir does not > exist but user passes --nomount, then we fail to pin the path (given > that the directory is not present), but the message returned will be "no > BPF file system found, not mounting it due to --nomount option", which > is confusing. > > Same note applies to the other function as well. > > > if (is_bpffs(dir)) > > > > /* nothing to do if already mounted */ > > > > @@ -277,11 +333,11 @@ int mount_bpffs_for_pin(const char *name, bool > > is_dir)> > > if (err) { > > > > err_str[ERR_MAX_LEN - 1] = '\0'; > > p_err("can't mount BPF file system to pin the object (%s): %s", > > We could also indicate the path where we tried to mount the bpffs, in > this message. Understood. I'll make these changes as well. > [...] > Other than the comments above, the patch works well, and the different > cases are much easier to follow than in v1, thanks! > > I've checked that the programs load as expected, the directories are > created (or not) as expected, and the bpffs is mounted (or not) as > expected, for all the cases I could think of, with the following > commands (copied here, for the record), all worked as I expected: That's really nice to hear. I'll incorporate the recommended changes and will send v3 soon. Thanks, Sahil
2024-03-14 21:16 UTC+0000 ~ Sahil <icegambit91@gmail.com> > Hi, > > On Wednesday, March 13, 2024 9:17:44 PM IST Quentin Monnet wrote: >> Thanks! Apologies for the delay. > > No worries! Thank you for the review. > >> [...] >> Note: you don't need the blank lines between the tags. >> [...] >> You can keep the changelog as part of the patch description. > > Got it. I'll keep this in mind when I submit v3. > >> [...] >> With all the checks and the potential directory creation, we could maybe >> rename this into "prepare_bpffs_dir()" or something like this? > > "prepare_bpffs_dir" is quite apt. If longer names are acceptable then I > would also recommend "prepare_and_mount_bpffs_dir" so it indicates > that it'll also mount the bpffs on the dir (when relevant) after performing > the checks. The length looks acceptable to me. I used "prepare" to summarise "create and mount", if you prefer "mount" explicitly then maybe "create_and_mount_bpffs_dir()"? There's no other preparation step as far as I remember so we should as well make it clear. > That's really nice to hear. I'll incorporate the recommended changes and > will send v3 soon. Thanks a lot, Quentin
Hi, Sorry for the delay in my response. I have made the relevant changes and have sent v3. Thanks, Sahil
diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c index cc6e6aae2447..d2746681200e 100644 --- a/tools/bpf/bpftool/common.c +++ b/tools/bpf/bpftool/common.c @@ -244,24 +244,80 @@ 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 mount_bpffs_on_dir(const char *dir_name) { char err_str[ERR_MAX_LEN]; - char *file; - char *dir; int err = 0; - if (is_dir && is_bpffs(name)) + if (is_bpffs(dir_name)) return err; - file = malloc(strlen(name) + 1); - if (!file) { + if (access(dir_name, F_OK) == -1) { + 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; + } + + free(temp_name); + + if (block_mount) { + p_err("no BPF file system found, not mounting it due to --nomount option"); + return -1; + } + + err = mkdir(dir_name, 0700); + if (err) { + p_err("failed to create dir (%s): %s", dir_name, strerror(errno)); + return err; + } + } else if (block_mount) { + p_err("no BPF file system found, not mounting it due to --nomount option"); + return -1; + } + + 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); + } + + return err; +} + +int mount_bpffs_given_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("bpf object can't be pinned since file (%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 */ @@ -277,11 +333,11 @@ int mount_bpffs_for_pin(const char *name, bool is_dir) 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); + file_name, err_str); } out_free: - free(file); + free(temp_name); return err; } @@ -289,7 +345,7 @@ int do_pin_fd(int fd, const char *name) { int err; - err = mount_bpffs_for_pin(name, false); + err = mount_bpffs_given_file(name); if (err) return err; diff --git a/tools/bpf/bpftool/iter.c b/tools/bpf/bpftool/iter.c index 6b0e5202ca7a..3911563dcc60 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, false); + err = mount_bpffs_given_file(path); if (err) goto close_link; diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h index b8bb08d10dec..20e06ad183ec 100644 --- a/tools/bpf/bpftool/main.h +++ b/tools/bpf/bpftool/main.h @@ -142,7 +142,8 @@ 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, bool is_dir); +int mount_bpffs_given_file(const char *file_name); +int mount_bpffs_on_dir(const char *dir_name); 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 9cb42a3366c0..09b5f0415a5e 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -1778,7 +1778,10 @@ static int load_with_options(int argc, char **argv, bool first_prog_only) goto err_close_obj; } - err = mount_bpffs_for_pin(pinfile, !first_prog_only); + if (first_prog_only) + err = mount_bpffs_given_file(pinfile); + else + err = mount_bpffs_on_dir(pinfile); if (err) goto err_close_obj; diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c index d573f2640d8e..bf50a99b2501 100644 --- a/tools/bpf/bpftool/struct_ops.c +++ b/tools/bpf/bpftool/struct_ops.c @@ -515,7 +515,7 @@ static int do_register(int argc, char **argv) if (argc == 1) linkdir = GET_ARG(); - if (linkdir && mount_bpffs_for_pin(linkdir, true)) { + if (linkdir && mount_bpffs_on_dir(linkdir)) { p_err("can't mount bpffs for pinning"); return -1; }
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> --- Changes since v1: - Split "mount_bpffs_for_pin" into two functions: - mount_bpffs_on_dir - mount_bpffs_given_file This is done to improve maintainability and readability. - Improve error messages - (mount_bpffs_given_file): If the file already exists, throw an error instead of waiting for bpf_obj_pin() to throw an error - Code style changes tools/bpf/bpftool/common.c | 78 +++++++++++++++++++++++++++++----- 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, 75 insertions(+), 15 deletions(-)