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. > 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). > 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. > +__printf(2, 3) void bpf_generic_log_write(struct bpf_generic_log *log, > + const char *fmt, ...) > +{ > + va_list args; > + > + if (!log->ubuf || !log->len_total) > + return; > + > + va_start(args, fmt); > + __bpf_generic_log_write(log, fmt, args); > + va_end(args); > +} [...]