On Wed, May 6, 2020 at 10:40 PM Yonghong Song <yhs@xxxxxx> 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@xxxxxx>
> ---
> 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@xxxxxx>
[...]
> + 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.
Same for pointers below, right?
> + params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt];
> +
> + fmt_cnt++;
> + memcpy_cnt++;
> + continue;
> + }
> +
[...]
