Re: [PATCH bpf-next v3 13/21] bpf: add bpf_seq_printf and bpf_seq_write helpers

Yonghong Song
2020-05-09T04:18:57Z

On 5/8/20 12:44 PM, Andrii Nakryiko wrote:
> On Wed, May 6, 2020 at 10:40 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Two helpers bpf_seq_printf and bpf_seq_write, are added for
>> writing data to the seq_file buffer.
>>
>> bpf_seq_printf supports common format string flag/width/type
>> fields so at least I can get identical results for
>> netlink and ipv6_route targets.
>>
>> For bpf_seq_printf and bpf_seq_write, return value -EOVERFLOW
>> specifically indicates a write failure due to overflow, which
>> means the object will be repeated in the next bpf invocation
>> if object collection stays the same. Note that if the object
>> collection is changed, depending how collection traversal is
>> done, even if the object still in the collection, it may not
>> be visited.
>>
>> bpf_seq_printf may return -EBUSY meaning that internal percpu
>> buffer for memory copy of strings or other pointees is
>> not available. Bpf program can return 1 to indicate it
>> wants the same object to be repeated. Right now, this should not
>> happen on no-RT kernels since migrate_disable(), which guards
>> bpf prog call, calls preempt_disable().
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> include/uapi/linux/bpf.h | 32 +++++-
>> kernel/trace/bpf_trace.c | 200 +++++++++++++++++++++++++++++++++
>> scripts/bpf_helpers_doc.py | 2 +
>> tools/include/uapi/linux/bpf.h | 32 +++++-
>> 4 files changed, 264 insertions(+), 2 deletions(-)
>>
>
> Was a bit surprised by behavior on failed memory read, I think it's
> important to emphasize and document this. But otherwise:
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
> [...]
>
>> + if (fmt[i] == 's') {
>> + /* try our best to copy */
>> + if (memcpy_cnt >= MAX_SEQ_PRINTF_MAX_MEMCPY) {
>> + err = -E2BIG;
>> + goto out;
>> + }
>> +
>> + bufs->buf[memcpy_cnt][0] = 0;
>> + strncpy_from_unsafe(bufs->buf[memcpy_cnt],
>> + (void *) (long) args[fmt_cnt],
>> + MAX_SEQ_PRINTF_STR_LEN);
>
> So the behavior is that we try to read string, but if it fails, we
> treat it as empty string? That needs to be documented, IMHO. My
> expectation was that entire printf would fail.

Let me return proper error. Currently, two possible errors may happen:
- user provide an invalid address, yes, an error should be returned
and we should not do anything
- user provide a valid address, but it needs page fault happening
to read the content. With current implementation,
strncpy_from_unsafe will return fail. Future sleepable
bpf program will help for this case, so an error means a
real address error.

>
> Same for pointers below, right?
>
>> + params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt];
>> +
>> + fmt_cnt++;
>> + memcpy_cnt++;
>> + continue;
>> + }
>> +
>
> [...]
>