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

From: Yonghong Song <yhs@fb.com>
To: Alexei Starovoitov <ast@fb.com>,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andriin@fb.com>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>,
	Networking <netdev@vger.kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next v3 13/21] bpf: add bpf_seq_printf and bpf_seq_write helpers
Date: Fri, 8 May 2020 23:04:17 -0700	[thread overview]
Message-ID: <9993230e-ce46-ba4d-7d5c-e5f8b3b8c078@fb.com> (raw)
In-Reply-To: <f78b0a02-9469-32c5-d8af-78335010660b@fb.com>

On 5/8/20 10:30 PM, Alexei Starovoitov wrote:
> On 5/8/20 9:18 PM, Yonghong Song wrote:
>>
>>
>> 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.
> 
> It matches what bpf_trace_printk() is doing.
> I suggest to defer any improvements to later patches.
> Both should be consistent.

Sure. We can do that.