diff mbox series

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

Message ID 20230708040750.72570-3-hffilwlqm@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series bpf: Introduce user log | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1737 this patch: 1737
netdev/cc_maintainers success CCed 18 of 18 maintainers
netdev/build_clang fail Errors and warnings before: 183 this patch: 186
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1736 this patch: 1736
netdev/checkpatch warning CHECK: No space is necessary after a cast WARNING: Unknown commit id '42c4e63ef36ea9b5ae50', maybe rebased or not pulled? WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline warning Was 2 now: 2
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-6 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on s390x with gcc

Commit Message

Leon Hwang July 8, 2023, 4:07 a.m. UTC
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(-)

Comments

Alexei Starovoitov July 10, 2023, 11:45 p.m. UTC | #1
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.
Leon Hwang July 11, 2023, 2:55 a.m. UTC | #2
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
diff mbox series

Patch

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;