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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 6 Jul 2023 17:36:32 +0800
From: Leon Hwang <hffilwlqm@...il.com>
To: Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Cc: john.fastabend@...il.com, andrii@...nel.org, martin.lau@...ux.dev,
 song@...nel.org, yhs@...com, kpsingh@...nel.org, sdf@...gle.com,
 haoluo@...gle.com, jolsa@...nel.org, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, hawk@...nel.org,
 tangyeechou@...il.com, kernel-patches-bot@...com, bpf@...r.kernel.org,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpf: Introduce bpf generic log


On 6/7/23 00:54, Daniel Borkmann wrote:
> 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@...il.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

I try to use BPF_ULOG_WRITE() successfully, but with a warning:


net/core/dev.c: In function 'bpf_xdp_link_log.isra.0':
net/core/dev.c:9093:1: warning: the frame size of 1056 bytes is larger 
than 1024 bytes [-Wframe-larger-than=]
  9093 | }
       |


How should we generalize the bpf_verifier_log infra with a smaller

BPF_VERIFIER_TMP_LOG_SIZE (1024 currently) ?


If to use bpf_log_once(attr, extack._msg), I think 
bpf_ulog_once(&attr->link_create.xdp.log, extack._msg)

is better. attr->link_create.xdp.log is struct bpf_generic_user_log.

Or bpf_xdp_link_log() is better to wrap log details.