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 <[email protected]>
---
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)
+
#endif /* _LINUX_BPF_H */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 60a9d59be..34fa33493 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 850494423..be56b153b 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 69a3e5446..e933809c0 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 60a9d59be..34fa33493 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 {
--
2.41.0
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 <[email protected]>
> ---
> 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?
> #endif /* _LINUX_BPF_H */
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 60a9d59be..34fa33493 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 */
You cannot add this here, this breaks user space, you would have to
ad this under a xdp specific section inside the union.
> 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 850494423..be56b153b 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 69a3e5446..e933809c0 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;
> }
Agree that this is a useful facility to have and propagate back here.
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index 60a9d59be..34fa33493 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 {
>
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 <[email protected]>
>> ---
>> 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)
>
>> #endif /* _LINUX_BPF_H */
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 60a9d59be..34fa33493 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 */
>
> You cannot add this here, this breaks user space, you would have to
> ad this under a xdp specific section inside the union.
Got it. I'll change it to avoid breaking user space.
>
>> 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 850494423..be56b153b 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 69a3e5446..e933809c0 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;
>> }
>
> Agree that this is a useful facility to have and propagate back here.
>
>> diff --git a/tools/include/uapi/linux/bpf.h
>> b/tools/include/uapi/linux/bpf.h
>> index 60a9d59be..34fa33493 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 {
>>
>
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>>>> ---
>>>> 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.