Re: [PATCH bpf-next] bpf: Introduce bpf generic log

[Daniel Borkmann <daniel@iogearbox.net>]

On 7/5/23 6:20 PM, Leon Hwang wrote:
> On 2023/7/5 22:39, Daniel Borkmann wrote:
>> On 7/5/23 3:20 PM, Leon Hwang 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@gmail.com>
>>> ---
>>>   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(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index f58895830..fd63f4a76 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -3077,4 +3077,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)
>>> +
>>
>> Could we generalize the bpf_verifier_log infra and reuse bpf_log() helper
>> instead of adding something new?
> 
> 
> Yes. It's possible to reuse the bpf_verifier_log infra and reuse bpf_log()
> helper. I'll try this way:
> 
> #define BPF_LOG_USER  BPF_LOG_LEVEL1     /* user log flag */
> 
> #define BPF_ULOG_WRITE(log_buf, log_size, fmt, ...) do {               \
>         const char *____fmt = (fmt);                                    \
>         struct bpf_verifier_log ____vlog;                               \
>         bpf_vlog_init(&____vlog, BPF_LOG_USER, log_buf, log_size);      \
>         bpf_log(&____vlog, ____fmt, ##__VA_ARGS__);                         \
> } while (0)

Could we hide all of this inside bpf_log() or better create a new bpf_log_once() wrapper,
passing in attr so we only need to use the latter w/o the extra macros?

Essentially what your bpf_xdp_link_log() is doing, just that bpf_log_once(attr, extack._msg)
would be generic and sufficient.

Thanks,
Daniel