[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <a945ff6e-1cd7-ef03-9ab4-3e5ab2a3bfa8@gmail.com>
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.
Powered by blists - more mailing lists