[PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil Siddiq]

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(-)

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",
-- 
2.44.0

Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Quentin Monnet]

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

Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil]

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

Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[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/

Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Quentin Monnet]

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/
> 
> 
> 

Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil]

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

Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Quentin Monnet]

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

Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil]

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