On 4/2/24 11:40 PM, Quentin Monnet wrote:
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);
nit: No need for extra ().
+ 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);
strdup() ?
+ 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);
nit: S_IRWXU
+ 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);
strdup()
+ 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.
+1, pls fix.
