Re: [PATCH bpf-next 2/4] bpf: implement dynptr copy kfuncs

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 4/25/25 19:20, Andrii Nakryiko wrote:
> On Fri, Apr 25, 2025 at 5:59 AM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> This patch introduces a new set of kfuncs for working with dynptrs in
>> BPF programs, enabling reading variable-length user or kernel data
>> into dynptr directly. To enable memory-safety, verifier allows only
>> constant-sized reads via existing bpf_probe_read_{user|kernel} etc.
>> kfuncs, dynptr-based kfuncs allow dynamically-sized reads without memory
>> safety shortcomings.
>>
>> The following kfuncs are introduced:
>> * `bpf_probe_read_kernel_dynptr()`: probes kernel-space data into a dynptr
>> * `bpf_probe_read_user_dynptr()`: probes user-space data into a dynptr
>> * `bpf_probe_read_kernel_str_dynptr()`: probes kernel-space string into
>> a dynptr
>> * `bpf_probe_read_user_str_dynptr()`: probes user-space string into a
>> dynptr
>> * `bpf_copy_from_user_dynptr()`: sleepable, copies user-space data into
>> a dynptr for the current task
>> * `bpf_copy_from_user_str_dynptr()`: sleepable, copies user-space string
>> into a dynptr for the current task
>> * `bpf_copy_from_user_task_dynptr()`: sleepable, copies user-space data
>> of the task into a dynptr
>> * `bpf_copy_from_user_task_str_dynptr()`: sleepable, copies user-space
>> string of the task into a dynptr
>>
>> The implementation is built on two generic functions:
>>   * __bpf_dynptr_copy
>>   * __bpf_dynptr_copy_str
>> These functions take function pointers as arguments, enabling the
>> copying of data from various sources, including both kernel and user
>> space. Notably, these indirect calls are typically inlined.
> you mean there are no indirect calls due to __bpf_dynptr_copy[_str]
> marked as __always_inline, right? We still call
> strncpy_from_user_nofault (as one example) as an underlying data
> reading step, right?
yes, exactly.
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>>   kernel/bpf/helpers.c     |   8 ++
>>   kernel/trace/bpf_trace.c | 199 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 207 insertions(+)
>>
> Logic and code structure look great, few nits around naming below, but
> LGTM overall.
>
> Reviewed-by: Andrii Nakryiko <andrii@kernel.org>
>
>> +static __always_inline int copy_kernel_data_nofault(void *dst, const void *unsafe_src,
>> +                                                   u32 size, struct task_struct *tsk)
>> +{
>> +       if (WARN_ON_ONCE(tsk))
>> +               return -EFAULT;
>> +
>> +       return copy_from_kernel_nofault(dst, unsafe_src, size);
>> +}
>> +
>> +static __always_inline int copy_user_data_str_nofault(void *dst, const void __user *unsafe_src,
> "user_data_str" is a bit mouthful, maybe just "copy_user_str_nofault"?
>
>> +                                                     u32 size, struct task_struct *tsk)
>> +{
>> +       if (WARN_ON_ONCE(tsk))
>> +               return -EFAULT;
>> +
>> +       return strncpy_from_user_nofault(dst, unsafe_src, size);
>> +}
>> +
>> +static __always_inline int copy_user_data_str_sleepable(void *dst, const void __user *unsafe_src,
>> +                                                       u32 size, struct task_struct *tsk)
>> +{
>> +       int ret;
>> +
>> +       if (unlikely(size == 0))
>> +               return 0;
>> +
>> +       if (tsk) {
>> +               ret = copy_remote_vm_str(tsk, (unsigned long)unsafe_src, dst, size, 0);
>> +       } else {
>> +               ret = strncpy_from_user(dst, unsafe_src, size - 1);
>> +               /* strncpy_from_user does not guarantee NUL termination */
>> +               if (ret >= 0)
>> +                       ((char *)dst)[ret] = '\0';
>> +       }
>> +
>> +       if (ret < 0)
>> +               return ret;
>> +       return ret + 1;
>> +}
>> +
>> +static __always_inline int copy_kernel_data_str_nofault(void *dst, const void *unsafe_src,
>> +                                                       u32 size, struct task_struct *tsk)
> ditto here and above, "data_str" is confusing. let's use "data" for
> fixed-size reads and "str" for zero-terminated strings?
Yes, makes sense
>
>> +{
>> +       if (WARN_ON_ONCE(tsk))
>> +               return -EFAULT;
>> +
>> +       return strncpy_from_kernel_nofault(dst, unsafe_src, size);
>> +}
>> +
>>   __bpf_kfunc_start_defs();
>>
>>   __bpf_kfunc int bpf_send_signal_task(struct task_struct *task, int sig, enum pid_type type,
> [...]