On Thu, Jan 20, 2022 at 2:46 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
a
> > + * wrapper of **access_process_vm**\ ().
> > + * Return
> > + * The number of bytes written to the buffer, or a negative error
> > + * in case of failure.
>
> wait, can it read less than *size* and return success?
>
> bpf_probe_read_kernel() returns:
>
> 0 on success, or a negative error in case of failure.
>
> Let's be consistent. Returning the number of read bytes makes more
> sense in cases when we don't know the amount of bytes to be actually
> read ahead of time (e.g., when reading zero-terminated strings).
>
> BTW, should we also add a C string reading helper as well, just like
> there is bpf_probe_read_user_str() and bpf_probe_read_user()?
That would be difficult. There is no suitable kernel api for that.
> Another thing, I think it's important to mention that this helper can
> be used only from sleepable BPF programs.
>
> And not to start the bikeshedding session, but we have
> bpf_copy_from_user(), wouldn't something like
> bpf_copy_from_user_{vm,process,remote}() be more in line and less
> surprising for BPF users. BTW, "access" implies writing just as much
> as reading, so using "access" in the sense of "read" seems wrong and
> confusing.
How about bpf_copy_from_user_task() ?
The task is the second to last argument, so the name fits ?
Especially if we call it this way it would be best to align
return codes with bpf_copy_from_user.
Adding memset() in case of failure is mandatory too.
I've missed this bit earlier.
The question is to decide what to do with
ret > 0 && ret < size condition.
Is it a failure and we should memset() the whole buffer and
return -EFAULT or memset only the leftover bytes and return 0?
I think the former is best to align with bpf_copy_from_user.
