| Message ID | 20230708040750.72570-2-hffilwlqm@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Introduce user log | expand |
On Fri, Jul 7, 2023 at 9:08 PM Leon Hwang <hffilwlqm@gmail.com> 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(-) > 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); > +} [...]
On 11/7/23 06:23, Andrii Nakryiko wrote: > On Fri, Jul 7, 2023 at 9:08 PM Leon Hwang <hffilwlqm@gmail.com> 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(-) >> > > 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
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 */ 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; +} + +__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); +} +EXPORT_SYMBOL_GPL(bpf_generic_log_write); diff --git a/net/core/dev.c b/net/core/dev.c index 69a3e544676c4..e933809c0a72f 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -9409,12 +9409,20 @@ static const struct bpf_link_ops bpf_xdp_link_lops = { .update_prog = bpf_xdp_link_update, }; +static inline void bpf_xdp_link_log(const union bpf_attr *attr, struct netlink_ext_ack *extack) +{ + const struct bpf_generic_user_log *ulog = &attr->link_create.log; + + BPF_GENERIC_ULOG_WRITE(ulog, extack->_msg); +} + int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog) { struct net *net = current->nsproxy->net_ns; struct bpf_link_primer link_primer; struct bpf_xdp_link *link; struct net_device *dev; + struct netlink_ext_ack extack; int err, fd; rtnl_lock(); @@ -9440,12 +9448,13 @@ int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog) goto unlock; } - err = dev_xdp_attach_link(dev, NULL, link); + err = dev_xdp_attach_link(dev, &extack, link); rtnl_unlock(); if (err) { link->dev = NULL; bpf_link_cleanup(&link_primer); + bpf_xdp_link_log(attr, &extack); goto out_put_dev; } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 60a9d59beeabb..34fa334938ba5 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/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 */ union { __u32 target_btf_id; /* btf_id of target to attach to */ struct {
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(-)