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

Commit Message

Leon Hwang Feb. 24, 2025, 3:33 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.

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. 24, 2025, 7:41 p.m. UTC | #1

On Mon, Feb 24, 2025 at 7:34 AM Leon Hwang <[e-mail]> wrote:
>
> @@ -3539,7 +3540,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);

I still don't like this uapi addition.

It only helps a rare corner case of freplace usage:
                /* If there is no saved target, or the specified target is
                 * different from the destination specified at load time, we
                 * need a new trampoline and a check for compatibility
                 */

If it was useful in more than one case we could consider it,
but uapi addition for a single rare use, is imo wrong trade off.

pw-bot: cr

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 dbd89c13dd328..970018ada1209 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3414,7 +3414,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;
@@ -3539,7 +3540,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;
@@ -3951,7 +3952,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)
@@ -5313,9 +5314,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))
@@ -5328,10 +5333,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:
@@ -5347,7 +5375,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:
@@ -5365,7 +5394,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:
@@ -5408,6 +5438,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);
@@ -5863,7 +5898,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);