| Message ID | 20240229130543.17491-1-icegambit91@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir | expand |
2024-02-29 13:05 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> > --- > 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
Hi, Thank you for the review. On Thursday, February 29, 2024 8:29:07 PM IST Quentin Monnet wrote: > [...] > 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. Sorry, I forgot to change the error message here. I'll change it. > > + 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. Got it. I ran the checkpatch script without --strict, so it didn't catch this. > > + } > > + > > + 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. Oh, I understand this now. The block_mount check should be done before any attempt to mount the bpffs. > 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. I agree. I am thinking of having two mount_bpffs_for_pin_* functions, one for dirs and one for files. These will handle the differences between dirs and files and they can both call a third (but static) mount_bpffs_for_pin where the code common to both scenarios will exist. The actual mounting and --nomount check can be done in this static function. > [...] > 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. Understood. I'll change this too. Thanks, Sahil
Hi, On Thursday, February 29, 2024 8:29:07 PM IST Quentin Monnet wrote: > [...] > 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. > I was working on these changes here, and I have got a question. In the description of the github issue [1], one scenario is when the given directory does not exist but its parent directory is bpffs. In this scenario no mounting should be done. But to check whether the parent dir is bpffs, the malloc and dirname will still have to be done. In the file subfunction too, the malloc and dirname will have to be done if the given file does not already exist. If my understanding above is right, should the mount_bpffs_for_pin() function still be split? Assuming that the function is split into two subfunctions, there's another question that I have got. > if (is_dir && is_bpffs(name)) > return err; The above condition was added in commit 2a36c26fe3b8 (patch submission [2]). If the function is to be split into two subfunctions for dirs and files, is it ok to remove the above function entirely in the file subfunction? If "is_bpffs(name)" returns false, then that could imply that the file exists and its parent dir is not bpffs, or that the file does not exist and no comment can be made on the parent dir. In either case the malloc and dirname will have to be done. On the other hand if the file exists and is part of the bpffs then this condition will allow the function to exit immediately without doing a malloc and dirname. But this can be determined without the condition as well, since the file being part of the bpffs implies that the dir will be bpffs. Thanks, Sahil [1] https://github.com/libbpf/bpftool/issues/100 [2] https://lore.kernel.org/bpf/1683197138-1894-1-git-send-email-yangpc@wangsu.com/
2024-03-01 20:28 UTC+0000 ~ Sahil <icegambit91@gmail.com> > Hi, > > On Thursday, February 29, 2024 8:29:07 PM IST Quentin Monnet wrote: >> [...] >> 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. >> > > I was working on these changes here, and I have got a question. In the > description of the github issue [1], one scenario is when the given directory > does not exist but its parent directory is bpffs. In this scenario no mounting > should be done. > > But to check whether the parent dir is bpffs, the malloc and dirname will still > have to be done. Yes, true > In the file subfunction too, the malloc and dirname will have to be done if the > given file does not already exist. > > If my understanding above is right, should the mount_bpffs_for_pin() function > still be split? Splitting the function was a suggestion, but you don't *have to* do it. What matters is the clarity of the resulting code, we want the function to be easy to follow and to not mix the file vs. directory paths too much (or then it's very easy to introduce bugs such as the existing one, or the missing --nomount check in your v1). Don't focus too much on malloc()/dirname() here, just make the logics easy to understand. > > Assuming that the function is split into two subfunctions, there's another > question that I have got. > >> if (is_dir && is_bpffs(name)) >> return err; > > The above condition was added in commit 2a36c26fe3b8 (patch submission [2]). > If the function is to be split into two subfunctions for dirs and files, is it ok to > remove the above function entirely in the file subfunction? If I understand correctly what you're asking, for files, "is_dir" would always evaluate to false so this check would be useless, wouldn't it? So yes we'd remove it. > > If "is_bpffs(name)" returns false, then that could imply that the file exists and its > parent dir is not bpffs, or that the file does not exist and no comment can be > made on the parent dir. In either case the malloc and dirname will have to be > done. > > On the other hand if the file exists Note: We handle the case where a directory exists, not when the file itself already exists. If the file exists we get an error when trying to pin the program. > and is part of the bpffs then this condition > will allow the function to exit immediately without doing a malloc and dirname. > But this can be determined without the condition as well, since the file being > part of the bpffs implies that the dir will be bpffs. > > Thanks, > Sahil > > [1] https://github.com/libbpf/bpftool/issues/100 > [2] https://lore.kernel.org/bpf/1683197138-1894-1-git-send-email-yangpc@wangsu.com/ > > >
Hi, Thank you for the reply. On Monday, March 4, 2024 7:20:40 PM IST Quentin Monnet wrote: > [...] > Splitting the function was a suggestion, but you don't *have to* do it. > What matters is the clarity of the resulting code, we want the function > to be easy to follow and to not mix the file vs. directory paths too > much (or then it's very easy to introduce bugs such as the existing one, > or the missing --nomount check in your v1). Don't focus too much on > malloc()/dirname() here, just make the logics easy to understand. Understood, I'll refactor it accordingly. > [...] > If I understand correctly what you're asking, for files, "is_dir" would > always evaluate to false so this check would be useless, wouldn't it? So > yes we'd remove it. This is the first part of my query. > > If "is_bpffs(name)" returns false, then that could imply that the file > > exists and its parent dir is not bpffs, or that the file does not exist > > and no comment can be made on the parent dir. In either case the malloc > > and dirname will have to be done. > > > > On the other hand if the file exists > > Note: We handle the case where a directory exists, not when the file > itself already exists. If the file exists we get an error when trying to > pin the program. Right. And this is related to the second part of my query. If the file already exists, an error should be thrown. But if it does not exist and the dir is not bpffs, we'll end up doing: is_bpffs(name) // in the condition mentioned above [...] dir_name = dirname(name); // get parent dir name is_bpffs(dir_name) // call is_bpffs again [...] So, in the "if" statement below: if (is_dir && is_bpffs(name)) will it be better to remove the second condition as well, and in lieu of that we could simply run dirname() and is_bpffs(dir_name) if the function gets a file as an argument? Thanks, Sahil
2024-03-04 20:34 UTC+0000 ~ Sahil <icegambit91@gmail.com> > Hi, > > Thank you for the reply. > > On Monday, March 4, 2024 7:20:40 PM IST Quentin Monnet wrote: >> [...] >> Splitting the function was a suggestion, but you don't *have to* do it. >> What matters is the clarity of the resulting code, we want the function >> to be easy to follow and to not mix the file vs. directory paths too >> much (or then it's very easy to introduce bugs such as the existing one, >> or the missing --nomount check in your v1). Don't focus too much on >> malloc()/dirname() here, just make the logics easy to understand. > > Understood, I'll refactor it accordingly. > >> [...] >> If I understand correctly what you're asking, for files, "is_dir" would >> always evaluate to false so this check would be useless, wouldn't it? So >> yes we'd remove it. > > This is the first part of my query. > >>> If "is_bpffs(name)" returns false, then that could imply that the file >>> exists and its parent dir is not bpffs, or that the file does not exist >>> and no comment can be made on the parent dir. In either case the malloc >>> and dirname will have to be done. >>> >>> On the other hand if the file exists >> >> Note: We handle the case where a directory exists, not when the file >> itself already exists. If the file exists we get an error when trying to >> pin the program. > > Right. And this is related to the second part of my query. If the file already > exists, an error should be thrown. But if it does not exist and the dir is not > bpffs, we'll end up doing: > > is_bpffs(name) // in the condition mentioned above > [...] > dir_name = dirname(name); // get parent dir name > is_bpffs(dir_name) // call is_bpffs again > [...] > > So, in the "if" statement below: > > if (is_dir && is_bpffs(name)) > > will it be better to remove the second condition as well, and in lieu of that > we could simply run dirname() and is_bpffs(dir_name) if the function gets > a file as an argument? Yes, we should remove this second condition too for files. Running "is_bpffs(name)" makes no sense as we know we'll have an error at this point. What we could do to improve the current code, however, is returning an error from mount_bpffs_for_pin() if the file exists, rather than mounting the bpffs and waiting for bpf_obj_pin() to return the error. This would prevent bpftool from mounting the bpffs when we already know the operation will fail. Quentin
Hi, On Tuesday, March 5, 2024 4:05:32 PM IST Quentin Monnet wrote: > [...] > Yes, we should remove this second condition too for files. Running > "is_bpffs(name)" makes no sense as we know we'll have an error at this > point. > > What we could do to improve the current code, however, is returning an > error from mount_bpffs_for_pin() if the file exists, rather than > mounting the bpffs and waiting for bpf_obj_pin() to return the error. > This would prevent bpftool from mounting the bpffs when we already know > the operation will fail. > > Quentin Sorry for the delay in replying. I have got a patch ready and have incorporated this as well. I need to run a few tests and then I'll be able to send a new patch. Thanks, Sahil
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", + name, err_str); + } + + return err; + } + 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); + 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",
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 Signed-off-by: Sahil Siddiq <icegambit91@gmail.com> --- tools/bpf/bpftool/common.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)