[bpf-next,v3,2/4] bpf: Improve error reporting for freplace attachment failure

Commit Message

Leon Hwang Feb. 17, 2025, 3:43 p.m. UTC

When a freplace program fails to attach to a target, the error message
lacks details, making debugging difficult. This patch enhances error
reporting by providing a log that explains why the attachment failed.

For example, if a freplace program tries to attach to a static function,
the log now includes:

libbpf: prog 'new_test_pkt_access': failed to attach to freplace: -EINVAL
libbpf: prog 'new_test_pkt_access': attach log: subprog_tail() is not a global function

Changes:

* Added verifier log to capture the log of freplace attachment failure.
* Updated bpf_tracing_prog_attach() to accept verifier log.
* Extended struct bpf_attr with a user-supplied log buffer for tracing
  programs.

This improves debugging by giving clear feedback when a freplace
attachment fails.

Signed-off-by: Leon Hwang <[e-mail]>
---
 include/uapi/linux/bpf.h |  2 ++
 kernel/bpf/syscall.c     | 51 +++++++++++++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 8 deletions(-)

Comments

Alexei Starovoitov Feb. 18, 2025, 3:01 a.m. UTC | #1

On Mon, Feb 17, 2025 at 7:44 AM Leon Hwang <[e-mail]> wrote:
>
> When a freplace program fails to attach to a target, the error message
> lacks details, making debugging difficult. This patch enhances error
> reporting by providing a log that explains why the attachment failed.

Agree that it lacks details...

> For example, if a freplace program tries to attach to a static function,
> the log now includes:
>
> libbpf: prog 'new_test_pkt_access': failed to attach to freplace: -EINVAL
> libbpf: prog 'new_test_pkt_access': attach log: subprog_tail() is not a global function

... but adding to uapi for a minor usability improvement...
not a long term path that is worth taking.
Especially since freplace is special. Users don't interact with it
directly. The interaction is typically done through libraries.
So this extra verbosity won't help users directly, but will
help people who write libraries. Nice, but no.

Leon Hwang Feb. 18, 2025, 3:29 a.m. UTC | #2

On 18/2/25 11:01, Alexei Starovoitov wrote:
> On Mon, Feb 17, 2025 at 7:44 AM Leon Hwang <[e-mail]> wrote:
>>
>> When a freplace program fails to attach to a target, the error message
>> lacks details, making debugging difficult. This patch enhances error
>> reporting by providing a log that explains why the attachment failed.
> 
> Agree that it lacks details...
> 
>> For example, if a freplace program tries to attach to a static function,
>> the log now includes:
>>
>> libbpf: prog 'new_test_pkt_access': failed to attach to freplace: -EINVAL
>> libbpf: prog 'new_test_pkt_access': attach log: subprog_tail() is not a global function
> 
> ... but adding to uapi for a minor usability improvement...
> not a long term path that is worth taking.
> Especially since freplace is special. Users don't interact with it
> directly. The interaction is typically done through libraries.
> So this extra verbosity won't help users directly, but will
> help people who write libraries. Nice, but no.

What if libbpf allows users to interact with it directly?

As for cilium/ebpf library, it will be able to provide the error message
to users like:

create link: subprog_tail() is not a global function

instead of:

create link: invalid argument

Thanks,
Leon

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index fff6cdb8d11a2..bea4d802d4463 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1759,6 +1759,8 @@  union bpf_attr {
 				 * accessible through bpf_get_attach_cookie() BPF helper
 				 */
 				__u64		cookie;
+				__aligned_u64	log_buf;	/* user supplied buffer */
+				__u32		log_size;	/* size of user buffer */
 			} tracing;
 			struct {
 				__u32		pf;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c420edbfb7c87..f41d1eea102a4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3412,7 +3412,8 @@  static const struct bpf_link_ops bpf_tracing_link_lops = {
 static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 				   int tgt_prog_fd,
 				   u32 btf_id,
-				   u64 bpf_cookie)
+				   u64 bpf_cookie,
+				   struct bpf_verifier_log *log)
 {
 	struct bpf_link_primer link_primer;
 	struct bpf_prog *tgt_prog = NULL;
@@ -3537,7 +3538,7 @@  static int bpf_tracing_prog_attach(struct bpf_prog *prog,
 		 */
 		struct bpf_attach_target_info tgt_info = {};
 
-		err = bpf_check_attach_target(NULL, prog, tgt_prog, btf_id,
+		err = bpf_check_attach_target(log, prog, tgt_prog, btf_id,
 					      &tgt_info);
 		if (err)
 			goto out_unlock;
@@ -3949,7 +3950,7 @@  static int bpf_raw_tp_link_attach(struct bpf_prog *prog,
 			tp_name = prog->aux->attach_func_name;
 			break;
 		}
-		return bpf_tracing_prog_attach(prog, 0, 0, 0);
+		return bpf_tracing_prog_attach(prog, 0, 0, 0, NULL);
 	case BPF_PROG_TYPE_RAW_TRACEPOINT:
 	case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
 		if (strncpy_from_user(buf, user_tp_name, sizeof(buf) - 1) < 0)
@@ -5311,9 +5312,13 @@  static int bpf_map_do_batch(const union bpf_attr *attr,
 }
 
 #define BPF_LINK_CREATE_LAST_FIELD link_create.uprobe_multi.pid
-static int link_create(union bpf_attr *attr, bpfptr_t uattr)
+static int link_create(union bpf_attr *attr, bpfptr_t uattr, u32 uattr_size)
 {
+	struct bpf_verifier_log *log;
+	u32 log_true_size, log_size;
 	struct bpf_prog *prog;
+	__aligned_u64 log_buf;
+	bool use_log;
 	int ret;
 
 	if (CHECK_ATTR(BPF_LINK_CREATE))
@@ -5326,10 +5331,33 @@  static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 	if (IS_ERR(prog))
 		return PTR_ERR(prog);
 
+	switch (prog->type) {
+	case BPF_PROG_TYPE_EXT:
+		log_buf = attr->link_create.tracing.log_buf;
+		log_size = attr->link_create.tracing.log_size;
+		use_log = true;
+		break;
+	default:
+		use_log = false;
+	}
+
+	if (use_log) {
+		log = kvzalloc(sizeof(*log), GFP_KERNEL);
+		if (!log) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		ret = bpf_vlog_init(log, BPF_LOG_FIXED,
+				    (char __user *) (unsigned long) log_buf,
+				    log_size);
+		if (ret)
+			goto out_free_log;
+	}
+
 	ret = bpf_prog_attach_check_attach_type(prog,
 						attr->link_create.attach_type);
 	if (ret)
-		goto out;
+		goto out_free_log;
 
 	switch (prog->type) {
 	case BPF_PROG_TYPE_CGROUP_SKB:
@@ -5345,7 +5373,8 @@  static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 		ret = bpf_tracing_prog_attach(prog,
 					      attr->link_create.target_fd,
 					      attr->link_create.target_btf_id,
-					      attr->link_create.tracing.cookie);
+					      attr->link_create.tracing.cookie,
+					      log);
 		break;
 	case BPF_PROG_TYPE_LSM:
 	case BPF_PROG_TYPE_TRACING:
@@ -5363,7 +5392,8 @@  static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 			ret = bpf_tracing_prog_attach(prog,
 						      attr->link_create.target_fd,
 						      attr->link_create.target_btf_id,
-						      attr->link_create.tracing.cookie);
+						      attr->link_create.tracing.cookie,
+						      NULL);
 		break;
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
 	case BPF_PROG_TYPE_SK_LOOKUP:
@@ -5406,6 +5436,11 @@  static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 		ret = -EINVAL;
 	}
 
+	if (ret < 0 && use_log)
+		(void) bpf_vlog_finalize(log, &log_true_size);
+out_free_log:
+	if (use_log)
+		kvfree(log);
 out:
 	if (ret < 0)
 		bpf_prog_put(prog);
@@ -5861,7 +5896,7 @@  static int __sys_bpf(enum bpf_cmd cmd, bpfptr_t uattr, unsigned int size)
 		err = bpf_map_do_batch(&attr, uattr.user, BPF_MAP_DELETE_BATCH);
 		break;
 	case BPF_LINK_CREATE:
-		err = link_create(&attr, uattr);
+		err = link_create(&attr, uattr, size);
 		break;
 	case BPF_LINK_UPDATE:
 		err = link_update(&attr);