> "On error dst buffer is zeroed out."? This is an explicit guarantee.
Will add to the docs.
> is there a point in calling access_process_vm() with size == 0? It
> would validate that get_task_mm() succeeds, but that's pretty much it?
> So maybe instead just exit early if size is zero? It will be also less
> convoluted logic:
>
> if (size == 0)
> return 0;
> if (access_process_vm(...)) {
> memset(0);
> return -EFAULT;
> }
> return 0;
Will do an explicit check for `size == 0`.
Note that we still need to check if the return value of
`access_process_vm` == `size` to see if we have a partial read.
> > Without the above change, using bpf_copy_from_user_task() will trigger
> > rcu warning and may produce incorrect result. One option is to put
> > the above in a preparation patch before introducing
> > bpf_copy_from_user_task() so we won't have bisecting issues.
>
> Sure, patch #1 for sleepable bpf_iter, patch #2 for the helper? I
> mean, it's not a big deal, but both seem to deserve their own focused
> patches.
I'll split this into 2 patches and place the bpf_iter patch first.
> I appreciate that existing helpers already do this and it's good to
> follow suit for consistency, but what is the rationale behind zeroing
> memory on failure?
I believe the intent behind this is for security/privacy reasons.
On error, we don't want to unintentionally leak partially read data.
Thanks everyone for the suggestions!
Kenny
