Re: [PATCH bpf-next 2/5] bpf: Add user memory access kfuncs for mm_struct

[bot+bpf-ci]

> Register both kfuncs with the common BPF_EVENTS kfunc set and mark them

This isn't a bug, but could the changelog be more precise here? The
kfuncs go into common_btf_ids, which is registered for
BPF_PROG_TYPE_UNSPEC, so they are reachable from any sleepable program
rather than from a tracing-specific set.

Relatedly, was gating them on CONFIG_BPF_EVENTS intentional? Nothing in
either function depends on tracing, and the analogous
bpf_copy_from_user_task*() kfuncs in kernel/bpf/helpers.c are available
unconditionally.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 6388b6b23e490..9ba3362832cd0 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -4931,6 +4931,8 @@ BTF_ID_FLAGS(func, bpf_iter_kmem_cache_destroy, KF_ITER_DESTROY | KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_local_irq_save)
>  BTF_ID_FLAGS(func, bpf_local_irq_restore)
>  #ifdef CONFIG_BPF_EVENTS
> +BTF_ID_FLAGS(func, bpf_copy_from_user_mm, KF_SLEEPABLE)
> +BTF_ID_FLAGS(func, bpf_copy_from_user_mm_str, KF_SLEEPABLE)
>  BTF_ID_FLAGS(func, bpf_probe_read_user_dynptr)
>  BTF_ID_FLAGS(func, bpf_probe_read_kernel_dynptr)
>  BTF_ID_FLAGS(func, bpf_probe_read_user_str_dynptr)
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 891897f8a1b35..8931b3b2fc9d7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -24,6 +24,7 @@
>  #include <linux/key.h>
>  #include <linux/namei.h>
>  #include <linux/file.h>
> +#include <linux/mm.h>
>
>  #include <net/bpf_sk_storage.h>
>

[ ... ]

> +__bpf_kfunc int bpf_copy_from_user_mm(void *dst, u32 dst__sz,
> +				      const void __user *unsafe_ptr__ign,
> +				      struct mm_struct *mm, u64 flags)
> +{
> +	int ret;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	if (unlikely(!dst__sz))
> +		return 0;
> +
> +	ret = access_remote_vm(mm, (unsigned long)unsafe_ptr__ign,
> +			       dst, dst__sz, 0);
> +	if (ret == dst__sz)
> +		return 0;
> +
> +	memset(dst, 0, dst__sz);
> +	return ret < 0 ? ret : -EFAULT;
> +}

[ ... ]

> +__bpf_kfunc int bpf_copy_from_user_mm_str(void *dst, u32 dst__sz,
> +					  const void __user *unsafe_ptr__ign,
> +					  struct mm_struct *mm, u64 flags)
> +{
> +	int ret;
> +
> +	if (unlikely(flags & ~BPF_F_PAD_ZEROS))
> +		return -EINVAL;
> +
> +	if (unlikely(!dst__sz))
> +		return 0;
> +
> +	ret = copy_remote_mm_str(mm, (unsigned long)unsafe_ptr__ign,
> +				 dst, dst__sz, 0);
> +	if (ret < 0)
> +		goto err;
> +
> +	if (flags & BPF_F_PAD_ZEROS)
> +		memset(dst + ret, 0, dst__sz - ret);
> +
> +	return ret + 1;
> +
> +err:
> +	if (flags & BPF_F_PAD_ZEROS)
> +		memset(dst, 0, dst__sz);
> +	return ret;
> +}

This isn't a bug, but since copy_remote_vm_str() in mm/memory.c is
already just a get_task_mm()/mmput() wrapper around copy_remote_mm_str(),
would it be cleaner to have bpf_copy_from_user_task_str() delegate to the
new mm-based kfunc (or share a static helper) rather than keeping two
near-identical copies of the flag/PAD_ZEROS/return-value logic in
different files?

The new copy also uses a 'goto err' where the existing one inlines the
error path, so the two now diverge in style too.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31488001112