Re: [PATCH bpf-next v2 1/2] bpf: Introduce bpf generic log

[Leon Hwang]

On 11/7/23 06:23, Andrii Nakryiko wrote:
> On Fri, Jul 7, 2023 at 9:08 PM Leon Hwang <hffilwlqm@xxxxxxxxx> wrote:
>>
>> Currently, excluding verifier, users are unable to obtain detailed error
>> information when issues occur in BPF syscall.
>>
>> To overcome this limitation, bpf generic log is introduced to provide
>> error details similar to the verifier. This enhancement will enable the
>> reporting of error details along with the corresponding errno in BPF
>> syscall.
>>
>> Essentially, bpf generic log functions as a mechanism similar to netlink,
>> enabling the transmission of error messages to user space. This
>> mechanism greatly enhances the usability of BPF syscall by allowing
>> users to access comprehensive error messages instead of relying solely
>> on errno.
>>
>> This patch specifically addresses the error reporting in dev_xdp_attach()
>> . With this patch, the error messages will be transferred to user space
>> like the netlink approach. Hence, users will be able to check the error
>> message along with the errno.
>>
>> Signed-off-by: Leon Hwang <hffilwlqm@xxxxxxxxx>
>> ---
>>  include/linux/bpf.h            | 30 ++++++++++++++++++++++++++++++
>>  include/uapi/linux/bpf.h       |  6 ++++++
>>  kernel/bpf/log.c               | 33 +++++++++++++++++++++++++++++++++
>>  net/core/dev.c                 | 11 ++++++++++-
>>  tools/include/uapi/linux/bpf.h |  6 ++++++
>>  5 files changed, 85 insertions(+), 1 deletion(-)
>>
> 
> Just curious, what's wrong with struct bpf_verifier_log for
> implementing "generic log"? bpf_log, bpf_vlog_reset, bpf_vlog_finalize
> are quite generic, I think. Why invent yet another structure? Existing
> code and struct support rotating log, if necessary.
> 

Sorry for the misguiding patch. This patch should not be submitted in v2.

>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 360433f14496a..7d2124a537943 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -3107,4 +3107,34 @@ static inline gfp_t bpf_memcg_flags(gfp_t flags)
>>         return flags;
>>  }
>>
>> +#define BPF_GENERIC_TMP_LOG_SIZE       256
>> +
>> +struct bpf_generic_log {
>> +       char            kbuf[BPF_GENERIC_TMP_LOG_SIZE];
>> +       char __user     *ubuf;
>> +       u32             len_used;
>> +       u32             len_total;
>> +};
>> +
>> +__printf(2, 3) void bpf_generic_log_write(struct bpf_generic_log *log,
>> +                       const char *fmt, ...);
>> +static inline void bpf_generic_log_init(struct bpf_generic_log *log,
>> +                       const struct bpf_generic_user_log *ulog)
>> +{
>> +       log->ubuf = (char __user *) (unsigned long) ulog->log_buf;
>> +       log->len_total = ulog->log_size;
>> +       log->len_used = 0;
>> +}
>> +
>> +#define BPF_GENERIC_LOG_WRITE(log, ulog, fmt, ...)     do {    \
>> +       const char *____fmt = (fmt);                            \
>> +       bpf_generic_log_init(log, ulog);                        \
>> +       bpf_generic_log_write(log, ____fmt, ##__VA_ARGS__);     \
>> +} while (0)
>> +
>> +#define BPF_GENERIC_ULOG_WRITE(ulog, fmt, ...) do {                    \
>> +       struct bpf_generic_log ____log;                                 \
>> +       BPF_GENERIC_LOG_WRITE(&____log, ulog, fmt, ##__VA_ARGS__);      \
>> +} while (0)
>> +
>>  #endif /* _LINUX_BPF_H */
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 60a9d59beeabb..34fa334938ba5 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1318,6 +1318,11 @@ struct bpf_stack_build_id {
>>         };
>>  };
>>
>> +struct bpf_generic_user_log {
>> +       __aligned_u64   log_buf;    /* user supplied buffer */
>> +       __u32           log_size;   /* size of user buffer */
>> +};
>> +
>>  #define BPF_OBJ_NAME_LEN 16U
>>
>>  union bpf_attr {
>> @@ -1544,6 +1549,7 @@ union bpf_attr {
>>                 };
>>                 __u32           attach_type;    /* attach type */
>>                 __u32           flags;          /* extra flags */
>> +               struct bpf_generic_user_log log; /* user log */
> 
> I think explicit triplet of log_level (should be log_flags, but too
> late, probably), log_size, and log_buf is less error prone and more in
> sync with other two commands that accept log (BPF_PROG_LOAD and
> BPF_BTF_LOAD).
> 

Better to keep same with BPF_PROG_LOAD and BPF_BTF_LOAD.

But considering the suggestion from Daniel Xu and Alexei Starovoitov,
it's better to add a tracepoint to expose the error message from
dev_xdp_attach(), without breaking uapi.

>>                 union {
>>                         __u32           target_btf_id;  /* btf_id of target to attach to */
>>                         struct {
>> diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
>> index 850494423530e..be56b153bbf0b 100644
>> --- a/kernel/bpf/log.c
>> +++ b/kernel/bpf/log.c
>> @@ -325,3 +325,36 @@ __printf(2, 3) void bpf_log(struct bpf_verifier_log *log,
>>         va_end(args);
>>  }
>>  EXPORT_SYMBOL_GPL(bpf_log);
>> +
>> +static inline void __bpf_generic_log_write(struct bpf_generic_log *log, const char *fmt,
>> +                                     va_list args)
>> +{
>> +       unsigned int n;
>> +
>> +       n = vscnprintf(log->kbuf, BPF_GENERIC_TMP_LOG_SIZE, fmt, args);
>> +
>> +       WARN_ONCE(n >= BPF_GENERIC_TMP_LOG_SIZE - 1,
>> +                 "bpf generic log truncated - local buffer too short\n");
>> +
>> +       n = min(log->len_total - log->len_used - 1, n);
>> +       log->kbuf[n] = '\0';
>> +
>> +       if (!copy_to_user(log->ubuf + log->len_used, log->kbuf, n + 1))
>> +               log->len_used += n;
>> +       else
>> +               log->ubuf = NULL;
>> +}
>> +
> 
> please see bpf_verifier_vlog() in kernel/bpf/log.c. We don't want to
> maintain another (even if light) version of it.
> 

Agree with you, we should not maintain multiple versions of it.

However, it will be replaced with tracepoint patch.

Thanks,
Leon