Re: [PATCH bpf-next] bpf: Clean code with bpf_copy_to_user

[Tao Chen]

在 2025/7/4 00:14, Yonghong Song 写道:
> On 7/3/25 9:03 AM, Tao Chen wrote:
> > 在 2025/7/3 23:35, Yonghong Song 写道:
> > > On 7/3/25 5:43 AM, Tao Chen wrote:
> > > > No logic change, just use bpf_copy_to_user to clean code.
> > > >
> > > > Signed-off-by: Tao Chen <chen.dylane@xxxxxxxxx>
> > > > ---
> > > >   kernel/bpf/syscall.c | 17 +++--------------
> > > >   1 file changed, 3 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > > index e6eea594f1c..ca152d36312 100644
> > > > --- a/kernel/bpf/syscall.c
> > > > +++ b/kernel/bpf/syscall.c
> > > > @@ -5208,21 +5208,10 @@ static int bpf_task_fd_query_copy(const 
> > > > union bpf_attr *attr,
> > > >               if (put_user(zero, ubuf))
> > > >                   return -EFAULT;
> > > > -        } else if (input_len >= len + 1) {
> > > > -            /* ubuf can hold the string with NULL terminator */
> > > > -            if (copy_to_user(ubuf, buf, len + 1))
> > > > -                return -EFAULT;
> > > >           } else {
> > > > -            /* ubuf cannot hold the string with NULL terminator,
> > > > -             * do a partial copy with NULL terminator.
> > > > -             */
> > > > -            char zero = '\0';
> > > > -
> > > > -            err = -ENOSPC;
> > > > -            if (copy_to_user(ubuf, buf, input_len - 1))
> > > > -                return -EFAULT;
> > > > -            if (put_user(zero, ubuf + input_len - 1))
> > > > -                return -EFAULT;
> > > > +            err = bpf_copy_to_user(ubuf, buf, input_len, len);
> > > > +            if (err)
> > > > +                return err;
> > > >           }
> > > >       }
> > >
> > > Actually, there is a return value change with this patch.
> > > bpf_copy_to_user() return returns -ENOSPC while the original
> > > implementation may return -EFAULT due to following code.
> > >
> > >          if (put_user(prog_id, &uattr->task_fd_query.prog_id) ||
> > >              put_user(fd_type, &uattr->task_fd_query.fd_type) ||
> > >              put_user(probe_offset, &uattr- 
> > > >task_fd_query.probe_offset) ||
> > >              put_user(probe_addr, &uattr->task_fd_query.probe_addr))
> > >                  return -EFAULT;
> > >
> > >          return err;
> >
> > You are right, maybe we can just use:
> >     err = bpf_copy_to_user(ubuf, buf, input_len, len);
> > and no return check
> > or move these put_user code to the front.
>
> Maybe do the following?
>
>     err = bpf_copy_to_user(ubuf, buf, input_len, len);
>     if (err && err != -ENOSPC)
>       return err;

Well, it looks better, i will change it in v2, thanks.

--
Best Regards
Tao Chen