kernel/trace/bpf_trace.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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(©, path, sizeof(*path));
- if (len < 0)
+ if (len < 0) {
+ memset(buf, 0, sz);
return len;
+ }
p = d_path(©, buf, sz);
if (IS_ERR(p)) {
+ memset(buf, 0, sz);
len = PTR_ERR(p);
} else {
len = buf + sz - p;
--
2.48.1On 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(©, path, sizeof(*path));
> - if (len < 0)
> + if (len < 0) {
> + memset(buf, 0, sz);
> return len;
> + }
>
> p = d_path(©, buf, sz);
> if (IS_ERR(p)) {
> + memset(buf, 0, sz);
> len = PTR_ERR(p);
> } else {
> len = buf + sz - p;
> --
> 2.48.1
>
>
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.
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.
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.
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!
在 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
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> [...]
© 2016 - 2025 Red Hat, Inc.