[PATCH bpf-next v2 0/2] bpf: Introduce user log

[PATCH bpf-next v2 0/2] bpf: Introduce user log

[Leon Hwang]

This series introduces bpf user log to transfer error message from
kernel space to user space when users provide buffer to receive the
error message.

Especially, when to attach XDP to device, it can transfer the error
message along with errno from dev_xdp_attach() to user space, if error
happens in dev_xdp_attach().

Leon Hwang (2):
  bpf: Introduce bpf generic log
  bpf: Introduce bpf user log

 include/linux/bpf.h            |  3 ++
 include/uapi/linux/bpf.h       |  8 ++++++
 kernel/bpf/log.c               | 52 ++++++++++++++++++++++++++++++++++
 net/core/dev.c                 |  4 ++-
 tools/include/uapi/linux/bpf.h |  8 ++++++
 5 files changed, 74 insertions(+), 1 deletion(-)


base-commit: 622f876ab3ced325fe3c2363c6e9c128b7e6c73a
-- 
2.41.0

[PATCH bpf-next v2 1/2] bpf: Introduce bpf generic log

[Leon Hwang]

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(-)

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 {
-- 
2.41.0

[PATCH bpf-next v2 2/2] bpf: Introduce bpf user log

[Leon Hwang]

Commit 42c4e63ef36ea9b5ae50 (bpf: Introduce bpf generic log) is
introduced to back propagate the error message in dev_xdp_attach() to
user space when users provide buffer to receive the error message.
But it breaks uapi and exposes many log details in header file.

To overcome the shortcomings, a) make the struct bpf_generic_user_log
as one field of the union in link_create of struct bpf_attr, b) hide
log details in log_ulog_once() and let its declaration in header file.

Furthermore, it's not a good idea to reuse bpf_verifier_log infra. Or, a
warning comes up:

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 | }
      |

As a result, a small kernel buffer, like 256 bytes, is preferred to
cache error message, then copy_to_user().

Then, unlike verifier to provide more and more error details, bpf user
log is only able to provide a simple error message limit 255 chars for
every time.

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 include/linux/bpf.h            | 31 ++-------------------------
 include/uapi/linux/bpf.h       |  4 +++-
 kernel/bpf/log.c               | 39 +++++++++++++++++++++++++---------
 net/core/dev.c                 |  9 +-------
 tools/include/uapi/linux/bpf.h |  4 +++-
 5 files changed, 38 insertions(+), 49 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7d2124a537943..36af7cc89cafc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3107,34 +3107,7 @@ 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)
+__printf(2, 3) void bpf_ulog_once(const struct bpf_generic_user_log *ulog,
+				  const char *fmt, ...);
 
 #endif /* _LINUX_BPF_H */
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 34fa334938ba5..8a458cfcd91bd 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1549,7 +1549,6 @@ 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 {
@@ -1585,6 +1584,9 @@ union bpf_attr {
 				__s32		priority;
 				__u32		flags;
 			} netfilter;
+			struct {
+				struct bpf_generic_user_log ulog; /* user log */
+			} xdp;
 		};
 	} link_create;
 
diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
index be56b153bbf0b..4d197b52ea207 100644
--- a/kernel/bpf/log.c
+++ b/kernel/bpf/log.c
@@ -326,15 +326,32 @@ __printf(2, 3) void bpf_log(struct bpf_verifier_log *log,
 }
 EXPORT_SYMBOL_GPL(bpf_log);
 
-static inline void __bpf_generic_log_write(struct bpf_generic_log *log, const char *fmt,
-				      va_list args)
+#define BPF_USER_TMP_LOG_SIZE	256
+
+struct bpf_user_log {
+	char		kbuf[BPF_USER_TMP_LOG_SIZE];
+	char __user	*ubuf;
+	u32		len_used;
+	u32		len_total;
+};
+
+static inline void bpf_ulog_init(struct bpf_user_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;
+}
+
+static inline void bpf_ulog_write(struct bpf_user_log *log,
+				  const char *fmt, va_list args)
 {
 	unsigned int n;
 
-	n = vscnprintf(log->kbuf, BPF_GENERIC_TMP_LOG_SIZE, fmt, args);
+	n = vscnprintf(log->kbuf, BPF_USER_TMP_LOG_SIZE, fmt, args);
 
-	WARN_ONCE(n >= BPF_GENERIC_TMP_LOG_SIZE - 1,
-		  "bpf generic log truncated - local buffer too short\n");
+	WARN_ONCE(n >= BPF_USER_TMP_LOG_SIZE - 1,
+		  "bpf user log line truncated - local buffer too short\n");
 
 	n = min(log->len_total - log->len_used - 1, n);
 	log->kbuf[n] = '\0';
@@ -345,16 +362,18 @@ static inline void __bpf_generic_log_write(struct bpf_generic_log *log, const ch
 		log->ubuf = NULL;
 }
 
-__printf(2, 3) void bpf_generic_log_write(struct bpf_generic_log *log,
-				     const char *fmt, ...)
+__printf(2, 3) void bpf_ulog_once(const struct bpf_generic_user_log *ulog,
+				  const char *fmt, ...)
 {
+	struct bpf_user_log log;
 	va_list args;
 
-	if (!log->ubuf || !log->len_total)
+	if (!ulog->log_buf || !ulog->log_size)
 		return;
 
+	bpf_ulog_init(&log, ulog);
 	va_start(args, fmt);
-	__bpf_generic_log_write(log, fmt, args);
+	bpf_ulog_write(&log, fmt, args);
 	va_end(args);
 }
-EXPORT_SYMBOL_GPL(bpf_generic_log_write);
+EXPORT_SYMBOL_GPL(bpf_ulog_once);
diff --git a/net/core/dev.c b/net/core/dev.c
index e933809c0a72f..311d3f3bc11b6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9409,13 +9409,6 @@ 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;
@@ -9454,7 +9447,7 @@ int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 	if (err) {
 		link->dev = NULL;
 		bpf_link_cleanup(&link_primer);
-		bpf_xdp_link_log(attr, &extack);
+		bpf_ulog_once(&attr->link_create.xdp.ulog, extack._msg);
 		goto out_put_dev;
 	}
 
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 34fa334938ba5..b81508bf758fa 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1549,7 +1549,6 @@ 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 {
@@ -1585,6 +1584,9 @@ union bpf_attr {
 				__s32		priority;
 				__u32		flags;
 			} netfilter;
+			struct {
+				struct bpf_generic_user_log ulog;  /* user log */
+			} xdp;
 		};
 	} link_create;
 
-- 
2.41.0

Re: [PATCH bpf-next v2 0/2] bpf: Introduce user log

[Daniel Xu]

Hi Leon,

On Sat, Jul 08, 2023 at 12:07:48PM +0800, Leon Hwang wrote:
> This series introduces bpf user log to transfer error message from
> kernel space to user space when users provide buffer to receive the
> error message.
> 
> Especially, when to attach XDP to device, it can transfer the error
> message along with errno from dev_xdp_attach() to user space, if error
> happens in dev_xdp_attach().

Have you considered adding a tracepoint instead? With some TP_printk()
stuff I think you can achieve a similar result without having to do
go through changing uapi.

> 
> Leon Hwang (2):
>   bpf: Introduce bpf generic log
>   bpf: Introduce bpf user log
> 
>  include/linux/bpf.h            |  3 ++
>  include/uapi/linux/bpf.h       |  8 ++++++
>  kernel/bpf/log.c               | 52 ++++++++++++++++++++++++++++++++++
>  net/core/dev.c                 |  4 ++-
>  tools/include/uapi/linux/bpf.h |  8 ++++++
>  5 files changed, 74 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 622f876ab3ced325fe3c2363c6e9c128b7e6c73a
> -- 
> 2.41.0
> 
> 

Thanks,
Daniel

Re: [PATCH bpf-next v2 0/2] bpf: Introduce user log

[Leon Hwang]

On 2023/7/9 06:02, Daniel Xu wrote:
> Hi Leon,
> 
> On Sat, Jul 08, 2023 at 12:07:48PM +0800, Leon Hwang wrote:
>> This series introduces bpf user log to transfer error message from
>> kernel space to user space when users provide buffer to receive the
>> error message.
>>
>> Especially, when to attach XDP to device, it can transfer the error
>> message along with errno from dev_xdp_attach() to user space, if error
>> happens in dev_xdp_attach().
> 
> Have you considered adding a tracepoint instead? With some TP_printk()
> stuff I think you can achieve a similar result without having to do
> go through changing uapi.

If just for dev_xdp_attach(), I think netlink approach is better than
tracepoint approach.

As for BPF syscall, error message along with errno through uapi is a
good UX, like "create link: invalid argument (Invalid XDP flags for BPF
link attachment)" when failed to attach XDP to a device. Hence, users
are able to know the error details instead of -EINVAL or "invalid
argument" only.

Furthermore, as for other BPF syscall subcommands, we are able to
provide error message along with errno by bpf_ulog_once(&attr->xxx.ulog,
"An error").

Re: [PATCH bpf-next v2 1/2] bpf: Introduce bpf generic log

[Andrii Nakryiko]

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);
> +}

[...]

Re: [PATCH bpf-next v2 2/2] bpf: Introduce bpf user log

[Alexei Starovoitov]

On Fri, Jul 7, 2023 at 9:08 PM Leon Hwang <hffilwlqm@gmail.com> wrote:
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 34fa334938ba5..8a458cfcd91bd 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1549,7 +1549,6 @@ 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 {
> @@ -1585,6 +1584,9 @@ union bpf_attr {
>                                 __s32           priority;
>                                 __u32           flags;
>                         } netfilter;
> +                       struct {
> +                               struct bpf_generic_user_log ulog; /* user log */
> +                       } xdp;

1.
You cannot break api in patch 1 and fix it in patch 2.

2.
libbpf side is missing.

3.
selftest is missing.

4.
bpf_vlog_finalize() should be used and error propagated back through
link_create.
Same api must be used: log_level, log_size, log_buf, log_true_size.

But considering all that I agree with Daniel Xu that
tracepoint would be better here.

Re: [PATCH bpf-next v2 2/2] bpf: Introduce bpf user log

[Leon Hwang]

On 11/7/23 07:45, Alexei Starovoitov wrote:
> On Fri, Jul 7, 2023 at 9:08 PM Leon Hwang <hffilwlqm@gmail.com> wrote:
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 34fa334938ba5..8a458cfcd91bd 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1549,7 +1549,6 @@ 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 {
>> @@ -1585,6 +1584,9 @@ union bpf_attr {
>>                                 __s32           priority;
>>                                 __u32           flags;
>>                         } netfilter;
>> +                       struct {
>> +                               struct bpf_generic_user_log ulog; /* user log */
>> +                       } xdp;
> 
> 1.
> You cannot break api in patch 1 and fix it in patch 2.
> 
> 2.
> libbpf side is missing.
> 
> 3.
> selftest is missing.
> 
> 4.
> bpf_vlog_finalize() should be used and error propagated back through
> link_create.
> Same api must be used: log_level, log_size, log_buf, log_true_size.
> 
> But considering all that I agree with Daniel Xu that
> tracepoint would be better here.

Sorry for missing 2&3.

Tracepoint is considered. I'll change it from user log to a tracepoint.

I'll submit tracepoint patch with libbpf&selftest patches later.

Thanks,
Leon

Re: [PATCH bpf-next v2 1/2] bpf: Introduce bpf generic log

[Leon Hwang]

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