[PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Tao Chen]

The bpf_d_path() function may fail. If it does,
clear the user buf, like bpf_probe_read etc.

Signed-off-by: Tao Chen <chen.dylane@linux.dev>
---
 kernel/trace/bpf_trace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 0998cbbb963..bb1003cb271 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -916,11 +916,14 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
 	 * potentially broken verifier.
 	 */
 	len = copy_from_kernel_nofault(&copy, path, sizeof(*path));
-	if (len < 0)
+	if (len < 0) {
+		memset(buf, 0, sz);
 		return len;
+	}
 
 	p = d_path(&copy, buf, sz);
 	if (IS_ERR(p)) {
+		memset(buf, 0, sz);
 		len = PTR_ERR(p);
 	} else {
 		len = buf + sz - p;
-- 
2.48.1

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Song Liu]

On Wed, Jun 11, 2025 at 8:49 AM Tao Chen <chen.dylane@linux.dev> wrote:
>
> The bpf_d_path() function may fail. If it does,
> clear the user buf, like bpf_probe_read etc.
>
> Signed-off-by: Tao Chen <chen.dylane@linux.dev>

Acked-by: Song Liu <song@kernel.org>

[...]

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Wed, 11 Jun 2025 23:48:58 +0800 you wrote:
> The bpf_d_path() function may fail. If it does,
> clear the user buf, like bpf_probe_read etc.
> 
> Signed-off-by: Tao Chen <chen.dylane@linux.dev>
> ---
>  kernel/trace/bpf_trace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Here is the summary with links:
  - [bpf-next] bpf: clear user buf when bpf_d_path failed
    https://git.kernel.org/bpf/bpf-next/c/3b55a9e6738b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Andrii Nakryiko]

On Wed, Jun 11, 2025 at 8:49 AM Tao Chen <chen.dylane@linux.dev> wrote:
>
> The bpf_d_path() function may fail. If it does,
> clear the user buf, like bpf_probe_read etc.
>

But that doesn't mean we *have to* do memset(0) for bpf_d_path(),
though. Especially given that path buffer can be pretty large (4KB).

Is there an issue you are trying to address with this, or is it more
of a consistency clean up? Note, that more or less recently we made
this zero filling behavior an option with an extra flag
(BPF_F_PAD_ZEROS) for newer APIs. And if anything, bpf_d_path() is
more akin to variable-sized string probing APIs rather than
fixed-sized bpf_probe_read* family.

In short, I feel like we should revert this and let users do
zero-filling, if they really need to. bpf_probe_read_kernel(dst, sz,
NULL) would do. But we should think about adding dynptr-based
bpf_dynptr_memset() API for cases when the size is not known
statically, IMO.


> Signed-off-by: Tao Chen <chen.dylane@linux.dev>
> ---
>  kernel/trace/bpf_trace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 0998cbbb963..bb1003cb271 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -916,11 +916,14 @@ BPF_CALL_3(bpf_d_path, struct path *, path, char *, buf, u32, sz)
>          * potentially broken verifier.
>          */
>         len = copy_from_kernel_nofault(&copy, path, sizeof(*path));
> -       if (len < 0)
> +       if (len < 0) {
> +               memset(buf, 0, sz);
>                 return len;
> +       }
>
>         p = d_path(&copy, buf, sz);
>         if (IS_ERR(p)) {
> +               memset(buf, 0, sz);
>                 len = PTR_ERR(p);
>         } else {
>                 len = buf + sz - p;
> --
> 2.48.1
>
>

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Alexei Starovoitov]

On Thu, Jun 12, 2025 at 2:29 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Jun 11, 2025 at 8:49 AM Tao Chen <chen.dylane@linux.dev> wrote:
> >
> > The bpf_d_path() function may fail. If it does,
> > clear the user buf, like bpf_probe_read etc.
> >
>
> But that doesn't mean we *have to* do memset(0) for bpf_d_path(),
> though. Especially given that path buffer can be pretty large (4KB).
>
> Is there an issue you are trying to address with this, or is it more
> of a consistency clean up? Note, that more or less recently we made
> this zero filling behavior an option with an extra flag
> (BPF_F_PAD_ZEROS) for newer APIs. And if anything, bpf_d_path() is
> more akin to variable-sized string probing APIs rather than
> fixed-sized bpf_probe_read* family.

All old helpers had this BPF_F_PAD_ZEROS behavior
(or rather should have had).
So it makes sense to zero in this helper too for consistency.
I don't share performance concerns. This is an error path.

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Andrii Nakryiko]

On Thu, Jun 12, 2025 at 2:40 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jun 12, 2025 at 2:29 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Jun 11, 2025 at 8:49 AM Tao Chen <chen.dylane@linux.dev> wrote:
> > >
> > > The bpf_d_path() function may fail. If it does,
> > > clear the user buf, like bpf_probe_read etc.
> > >
> >
> > But that doesn't mean we *have to* do memset(0) for bpf_d_path(),
> > though. Especially given that path buffer can be pretty large (4KB).
> >
> > Is there an issue you are trying to address with this, or is it more
> > of a consistency clean up? Note, that more or less recently we made
> > this zero filling behavior an option with an extra flag
> > (BPF_F_PAD_ZEROS) for newer APIs. And if anything, bpf_d_path() is
> > more akin to variable-sized string probing APIs rather than
> > fixed-sized bpf_probe_read* family.
>
> All old helpers had this BPF_F_PAD_ZEROS behavior
> (or rather should have had).
> So it makes sense to zero in this helper too for consistency.
> I don't share performance concerns. This is an error path.

It's just a bizarre behavior as it stands right now.

On error, you'll have a zeroed out buffer, OK, good so far.

On success, though, you'll have a buffer where first N bytes are
filled out with good path information, but then the last sizeof(buf) -
N bytes would be, effectively, garbage.

All in all, you can't use that buffer as a key for hashmap looking
(because of leftover non-zeroed bytes at the end), yet on error we
still zero out bytes for no apparently useful reason.

And then for the bpf_path_d_path(). What do we do about that one? It
doesn't have zeroing out either in the error path, nor in the success
path. So just more inconsistency all around.

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Alexei Starovoitov]

On Thu, Jun 12, 2025 at 4:27 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jun 12, 2025 at 2:40 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Jun 12, 2025 at 2:29 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Jun 11, 2025 at 8:49 AM Tao Chen <chen.dylane@linux.dev> wrote:
> > > >
> > > > The bpf_d_path() function may fail. If it does,
> > > > clear the user buf, like bpf_probe_read etc.
> > > >
> > >
> > > But that doesn't mean we *have to* do memset(0) for bpf_d_path(),
> > > though. Especially given that path buffer can be pretty large (4KB).
> > >
> > > Is there an issue you are trying to address with this, or is it more
> > > of a consistency clean up? Note, that more or less recently we made
> > > this zero filling behavior an option with an extra flag
> > > (BPF_F_PAD_ZEROS) for newer APIs. And if anything, bpf_d_path() is
> > > more akin to variable-sized string probing APIs rather than
> > > fixed-sized bpf_probe_read* family.
> >
> > All old helpers had this BPF_F_PAD_ZEROS behavior
> > (or rather should have had).
> > So it makes sense to zero in this helper too for consistency.
> > I don't share performance concerns. This is an error path.
>
> It's just a bizarre behavior as it stands right now.
>
> On error, you'll have a zeroed out buffer, OK, good so far.
>
> On success, though, you'll have a buffer where first N bytes are
> filled out with good path information, but then the last sizeof(buf) -
> N bytes would be, effectively, garbage.
>
> All in all, you can't use that buffer as a key for hashmap looking
> (because of leftover non-zeroed bytes at the end), yet on error we
> still zero out bytes for no apparently useful reason.
>
> And then for the bpf_path_d_path(). What do we do about that one? It
> doesn't have zeroing out either in the error path, nor in the success
> path. So just more inconsistency all around.

Consistency with bpf_path_d_path() kfunc is indeed missing.

Ok, since you insist, dropped this patch, and force pushed.

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Andrii Nakryiko]

On Thu, Jun 12, 2025 at 4:56 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jun 12, 2025 at 4:27 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Jun 12, 2025 at 2:40 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, Jun 12, 2025 at 2:29 PM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > On Wed, Jun 11, 2025 at 8:49 AM Tao Chen <chen.dylane@linux.dev> wrote:
> > > > >
> > > > > The bpf_d_path() function may fail. If it does,
> > > > > clear the user buf, like bpf_probe_read etc.
> > > > >
> > > >
> > > > But that doesn't mean we *have to* do memset(0) for bpf_d_path(),
> > > > though. Especially given that path buffer can be pretty large (4KB).
> > > >
> > > > Is there an issue you are trying to address with this, or is it more
> > > > of a consistency clean up? Note, that more or less recently we made
> > > > this zero filling behavior an option with an extra flag
> > > > (BPF_F_PAD_ZEROS) for newer APIs. And if anything, bpf_d_path() is
> > > > more akin to variable-sized string probing APIs rather than
> > > > fixed-sized bpf_probe_read* family.
> > >
> > > All old helpers had this BPF_F_PAD_ZEROS behavior
> > > (or rather should have had).
> > > So it makes sense to zero in this helper too for consistency.
> > > I don't share performance concerns. This is an error path.
> >
> > It's just a bizarre behavior as it stands right now.
> >
> > On error, you'll have a zeroed out buffer, OK, good so far.
> >
> > On success, though, you'll have a buffer where first N bytes are
> > filled out with good path information, but then the last sizeof(buf) -
> > N bytes would be, effectively, garbage.
> >
> > All in all, you can't use that buffer as a key for hashmap looking
> > (because of leftover non-zeroed bytes at the end), yet on error we
> > still zero out bytes for no apparently useful reason.
> >
> > And then for the bpf_path_d_path(). What do we do about that one? It
> > doesn't have zeroing out either in the error path, nor in the success
> > path. So just more inconsistency all around.
>
> Consistency with bpf_path_d_path() kfunc is indeed missing.
>
> Ok, since you insist, dropped this patch, and force pushed.

Great, thank you!

Re: [PATCH bpf-next] bpf: clear user buf when bpf_d_path failed

[Tao Chen]

在 2025/6/13 08:06, Andrii Nakryiko 写道:
> On Thu, Jun 12, 2025 at 4:56 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Thu, Jun 12, 2025 at 4:27 PM Andrii Nakryiko
>> <andrii.nakryiko@gmail.com> wrote:
>>>
>>> On Thu, Jun 12, 2025 at 2:40 PM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> On Thu, Jun 12, 2025 at 2:29 PM Andrii Nakryiko
>>>> <andrii.nakryiko@gmail.com> wrote:
>>>>>
>>>>> On Wed, Jun 11, 2025 at 8:49 AM Tao Chen <chen.dylane@linux.dev> wrote:
>>>>>>
>>>>>> The bpf_d_path() function may fail. If it does,
>>>>>> clear the user buf, like bpf_probe_read etc.
>>>>>>
>>>>>
>>>>> But that doesn't mean we *have to* do memset(0) for bpf_d_path(),
>>>>> though. Especially given that path buffer can be pretty large (4KB).
>>>>>
>>>>> Is there an issue you are trying to address with this, or is it more
>>>>> of a consistency clean up? Note, that more or less recently we made
>>>>> this zero filling behavior an option with an extra flag
>>>>> (BPF_F_PAD_ZEROS) for newer APIs. And if anything, bpf_d_path() is
>>>>> more akin to variable-sized string probing APIs rather than
>>>>> fixed-sized bpf_probe_read* family.
>>>>
>>>> All old helpers had this BPF_F_PAD_ZEROS behavior
>>>> (or rather should have had).
>>>> So it makes sense to zero in this helper too for consistency.
>>>> I don't share performance concerns. This is an error path.
>>>
>>> It's just a bizarre behavior as it stands right now.
>>>
>>> On error, you'll have a zeroed out buffer, OK, good so far.
>>>
>>> On success, though, you'll have a buffer where first N bytes are
>>> filled out with good path information, but then the last sizeof(buf) -
>>> N bytes would be, effectively, garbage.
>>>
>>> All in all, you can't use that buffer as a key for hashmap looking
>>> (because of leftover non-zeroed bytes at the end), yet on error we
>>> still zero out bytes for no apparently useful reason.
>>>
>>> And then for the bpf_path_d_path(). What do we do about that one? It
>>> doesn't have zeroing out either in the error path, nor in the success
>>> path. So just more inconsistency all around.
>>
>> Consistency with bpf_path_d_path() kfunc is indeed missing.
>>
>> Ok, since you insist, dropped this patch, and force pushed.
> 
> Great, thank you!

The changes in this patch are relatively simple, but the discussion 
between the two of you is more meaningful to me. I agree with Andrii's 
point of view. Thank you both for discussing this patch.

-- 
Best Regards
Tao Chen