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

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

[Leon Hwang]

This patch series improves error reporting for BPF_LINK_CREATE when
attaching freplace programs. Inspired by the discussion in
"[PATCH bpf-next v2] bpf: Add bpf_check_attach_target_with_klog method to
output failure logs to kernel"[0], this series enhances that freplace
attachment failure returns meaningful logs to userspace, aiding debugging.

Patch breakdown:
1. bpf, verifier: Add missing newline of bpf_log in bpf_check_attach_target
    * Add the missing newline in
      bpf_log(log, "Target program bound device mismatch").
2. bpf: Improve error reporting for freplace attachment failure
    * Extends BPF_LINK_CREATE to report detailed log.
3. bpf, libbpf: Capture error message of freplace attachment failure
    * Modifies libbpf to capture freplace attachment failure log.
4. selftests/bpf: Add test case for freplace attachment failure log
    * Introduces a selftest to validate error reporting.

Links:
[0] https://lore.kernel.org/bpf/CAEf4BzbbyojuFSS7xQ3+jZb=dHzOaZfMbtT+WnypW2LPwOUwRw@mail.gmail.com/

Changes:
v3 -> v4:
  * Add libbpf API bpf_program__attach_freplace_opts() to use users'
    supplied log buffer.

v2: https://lore.kernel.org/bpf/20240725051511.57112-1-me@manjusaka.me/
v2 -> v3:
  * Address comment from Andrii:
    * Report back the reason for declining freplace attachment instead of
      logging in dmesg.

Leon Hwang (4):
  bpf, verifier: Add missing newline of bpf_log in
    bpf_check_attach_target
  bpf: Improve error reporting for freplace attachment failure
  bpf, libbpf: Capture error message of freplace attachment failure
  selftests/bpf: Add test case for freplace attachment failure log

 include/uapi/linux/bpf.h                      |  2 +
 kernel/bpf/syscall.c                          | 51 ++++++++++++++++---
 kernel/bpf/verifier.c                         |  2 +-
 tools/include/uapi/linux/bpf.h                |  2 +
 tools/lib/bpf/bpf.c                           |  6 ++-
 tools/lib/bpf/bpf.h                           |  2 +
 tools/lib/bpf/libbpf.c                        | 29 +++++++++--
 tools/lib/bpf/libbpf.h                        | 14 +++++
 tools/lib/bpf/libbpf.map                      |  1 +
 .../bpf/prog_tests/tracing_link_attach_log.c  | 48 +++++++++++++++++
 10 files changed, 143 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_link_attach_log.c

-- 
2.47.1

[PATCH bpf-next v4 1/4] bpf, verifier: Add missing newline of bpf_log in bpf_check_attach_target

[Leon Hwang]

Add the missing newline in
bpf_log(log, "Target program bound device mismatch").

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5c9b7464ec2c9..6801662f1dd28 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -22701,7 +22701,7 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
 
 		if (bpf_prog_is_dev_bound(prog->aux) &&
 		    !bpf_prog_dev_bound_match(prog, tgt_prog)) {
-			bpf_log(log, "Target program bound device mismatch");
+			bpf_log(log, "Target program bound device mismatch\n");
 			return -EINVAL;
 		}
 
-- 
2.47.1

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

[Leon Hwang]

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 <leon.hwang@linux.dev>
---
 include/uapi/linux/bpf.h |  2 ++
 kernel/bpf/syscall.c     | 51 +++++++++++++++++++++++++++++++++-------
 2 files changed, 45 insertions(+), 8 deletions(-)

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);
-- 
2.47.1

[PATCH bpf-next v4 3/4] bpf, libbpf: Capture error message of freplace attachment failure

[Leon Hwang]

To improve debugging, this patch captures the log of
bpf_check_attach_target() when a freplace program fails to attach. Users
can supply a buffer to receive the log.

Changes:

* Extended bpf_attr and bpf_link_create_opts to include a log buffer for
  tracing.
* Updated bpf_link_create() to handle log buffer properly.
* Add bpf_program__attach_freplace_opts() to use users' supplied log
  buffer.

This helps diagnose freplace attachment failures more efficiently.

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 tools/include/uapi/linux/bpf.h |  2 ++
 tools/lib/bpf/bpf.c            |  6 +++++-
 tools/lib/bpf/bpf.h            |  2 ++
 tools/lib/bpf/libbpf.c         | 29 +++++++++++++++++++++++++----
 tools/lib/bpf/libbpf.h         | 14 ++++++++++++++
 tools/lib/bpf/libbpf.map       |  1 +
 6 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index fff6cdb8d11a2..bea4d802d4463 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/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/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index 359f73ead6137..cd422ecd53ae2 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -741,7 +741,7 @@ int bpf_link_create(int prog_fd, int target_fd,
 	if (iter_info_len || target_btf_id) {
 		if (iter_info_len && target_btf_id)
 			return libbpf_err(-EINVAL);
-		if (!OPTS_ZEROED(opts, target_btf_id))
+		if (!OPTS_ZEROED(opts, tracing))
 			return libbpf_err(-EINVAL);
 	}
 
@@ -753,6 +753,8 @@ int bpf_link_create(int prog_fd, int target_fd,
 
 	if (target_btf_id) {
 		attr.link_create.target_btf_id = target_btf_id;
+		attr.link_create.tracing.log_buf = ptr_to_u64(OPTS_GET(opts, tracing.log_buf, 0));
+		attr.link_create.tracing.log_size = OPTS_GET(opts, tracing.log_size, 0);
 		goto proceed;
 	}
 
@@ -794,6 +796,8 @@ int bpf_link_create(int prog_fd, int target_fd,
 	case BPF_MODIFY_RETURN:
 	case BPF_LSM_MAC:
 		attr.link_create.tracing.cookie = OPTS_GET(opts, tracing.cookie, 0);
+		attr.link_create.tracing.log_buf = 0;
+		attr.link_create.tracing.log_size = 0;
 		if (!OPTS_ZEROED(opts, tracing))
 			return libbpf_err(-EINVAL);
 		break;
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 435da95d20589..c76222617c18c 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -421,6 +421,8 @@ struct bpf_link_create_opts {
 		} uprobe_multi;
 		struct {
 			__u64 cookie;
+			const char *log_buf;
+			size_t log_size;
 		} tracing;
 		struct {
 			__u32 pf;
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6df258912e1ec..f0c42669ec1e1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -12837,10 +12837,11 @@ bpf_program__attach_netkit(const struct bpf_program *prog, int ifindex,
 	return bpf_program_attach_fd(prog, ifindex, "netkit", &link_create_opts);
 }
 
-struct bpf_link *bpf_program__attach_freplace(const struct bpf_program *prog,
-					      int target_fd,
-					      const char *attach_func_name)
+static struct bpf_link *bpf_program_attach_freplace(const struct bpf_freplace_opts *opts)
 {
+	const char *attach_func_name = opts->attach_func_name;
+	const struct bpf_program *prog = opts->prog;
+	int target_fd = opts->target_prog_fd;
 	int btf_id;
 
 	if (!!target_fd != !!attach_func_name) {
@@ -12863,7 +12864,8 @@ struct bpf_link *bpf_program__attach_freplace(const struct bpf_program *prog,
 			return libbpf_err_ptr(btf_id);
 
 		target_opts.target_btf_id = btf_id;
-
+		target_opts.tracing.log_buf = opts->log_buf;
+		target_opts.tracing.log_size = opts->log_buf_size;
 		return bpf_program_attach_fd(prog, target_fd, "freplace",
 					     &target_opts);
 	} else {
@@ -12874,6 +12876,25 @@ struct bpf_link *bpf_program__attach_freplace(const struct bpf_program *prog,
 	}
 }
 
+struct bpf_link *bpf_program__attach_freplace(const struct bpf_program *prog,
+					      int target_fd,
+					      const char *attach_func_name)
+{
+	LIBBPF_OPTS(bpf_freplace_opts, opts,
+		    .prog = prog,
+		    .target_prog_fd = target_fd,
+		    .attach_func_name = attach_func_name,
+	);
+
+	return bpf_program_attach_freplace(&opts);
+}
+
+struct bpf_link *
+bpf_program__attach_freplace_opts(const struct bpf_freplace_opts *opts)
+{
+	return bpf_program_attach_freplace(opts);
+}
+
 struct bpf_link *
 bpf_program__attach_iter(const struct bpf_program *prog,
 			 const struct bpf_iter_attach_opts *opts)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 3020ee45303a0..8de595da7e7d7 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -819,6 +819,20 @@ LIBBPF_API struct bpf_link *
 bpf_program__attach_freplace(const struct bpf_program *prog,
 			     int target_fd, const char *attach_func_name);
 
+struct bpf_freplace_opts {
+	size_t sz; /* size of this struct for forward/backward compatibility */
+
+	/* freplace bpf prog */
+	const struct bpf_program *prog;
+	int target_prog_fd;
+	const char *attach_func_name;
+	/* buffer to receive error message when fail to bpf_check_attach_target */
+	const char *log_buf;
+	size_t log_buf_size;
+};
+LIBBPF_API struct bpf_link *
+bpf_program__attach_freplace_opts(const struct bpf_freplace_opts *opts);
+
 struct bpf_netfilter_opts {
 	/* size of this struct, for forward/backward compatibility */
 	size_t sz;
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index b5a838de6f47c..5088f8af51dd6 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -438,4 +438,5 @@ LIBBPF_1.6.0 {
 		bpf_linker__new_fd;
 		btf__add_decl_attr;
 		btf__add_type_attr;
+		bpf_program__attach_freplace_opts;
 } LIBBPF_1.5.0;
-- 
2.47.1

[PATCH bpf-next v4 4/4] selftests/bpf: Add test case for freplace attachment failure log

[Leon Hwang]

This patch adds a selftest to verify that freplace attachment failure
produces meaningful log.

cd tools/testing/selftests/bpf/; ./test_progs -t attach_log
115     freplace_attach_log:OK
Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
 .../bpf/prog_tests/tracing_link_attach_log.c  | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/tracing_link_attach_log.c

diff --git a/tools/testing/selftests/bpf/prog_tests/tracing_link_attach_log.c b/tools/testing/selftests/bpf/prog_tests/tracing_link_attach_log.c
new file mode 100644
index 0000000000000..92f770966f8cd
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/tracing_link_attach_log.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "tailcall_bpf2bpf1.skel.h"
+#include "freplace_global_func.skel.h"
+
+void test_freplace_attach_log(void)
+{
+	struct freplace_global_func *freplace_skel = NULL;
+	struct tailcall_bpf2bpf1 *tailcall_skel = NULL;
+	struct bpf_link *freplace_link = NULL;
+	struct bpf_program *prog;
+	char log_buf[64];
+	int err, prog_fd;
+	LIBBPF_OPTS(bpf_freplace_opts, freplace_opts,
+		    .log_buf = log_buf,
+		    .log_buf_size = sizeof(log_buf),
+	);
+
+	tailcall_skel = tailcall_bpf2bpf1__open_and_load();
+	if (!ASSERT_OK_PTR(tailcall_skel, "tailcall_bpf2bpf1__open_and_load"))
+		return;
+
+	freplace_skel = freplace_global_func__open();
+	if (!ASSERT_OK_PTR(freplace_skel, "freplace_global_func__open"))
+		goto out;
+
+	prog = freplace_skel->progs.new_test_pkt_access;
+	prog_fd = bpf_program__fd(tailcall_skel->progs.entry);
+	err = bpf_program__set_attach_target(prog, prog_fd, "entry");
+	if (!ASSERT_OK(err, "bpf_program__set_attach_target"))
+		goto out;
+
+	err = freplace_global_func__load(freplace_skel);
+	if (!ASSERT_OK(err, "freplace_global_func__load"))
+		goto out;
+
+	freplace_opts.prog = prog;
+	freplace_opts.target_prog_fd = prog_fd;
+	freplace_opts.attach_func_name = "subprog_tail";
+	freplace_link = bpf_program__attach_freplace_opts(&freplace_opts);
+	ASSERT_ERR_PTR(freplace_link, "bpf_program__attach_freplace");
+	ASSERT_STREQ(log_buf, "subprog_tail() is not a global function\n", "log_buf");
+
+out:
+	bpf_link__destroy(freplace_link);
+	freplace_global_func__destroy(freplace_skel);
+	tailcall_bpf2bpf1__destroy(tailcall_skel);
+}
-- 
2.47.1

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

[Alexei Starovoitov]

On Mon, Feb 24, 2025 at 7:34 AM Leon Hwang <leon.hwang@linux.dev> 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

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

[Andrii Nakryiko]

On Mon, Feb 24, 2025 at 11:41 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Feb 24, 2025 at 7:34 AM Leon Hwang <leon.hwang@linux.dev> 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.

Agreed. I think the idea of verbose log is useful for bpf() syscall,
given how complicated some of its conditions are. But it should be
done more generically, ideally at syscall (or at least the entire BPF
command) level, not for one particular kind of link.

>
> pw-bot: cr

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

[Leon Hwang]

On 2025/2/25 03:41, Alexei Starovoitov wrote:
> On Mon, Feb 24, 2025 at 7:34 AM Leon Hwang <leon.hwang@linux.dev> 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.
> 

Got it.

I'm planning to implement a restrict version of
"bpf: make tracing program support multi-link"[0]. With log buffer, it
will be helpful to report the reason for declining attaching, especially
to report the tracee info that causes the attachment failure.

[0]
https://lore.kernel.org/bpf/20240311093526.1010158-1-dongmenglong.8@bytedance.com/

Thanks,
Leon

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

[Leon Hwang]

On 2025/2/25 06:08, Andrii Nakryiko wrote:
> On Mon, Feb 24, 2025 at 11:41 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Mon, Feb 24, 2025 at 7:34 AM Leon Hwang <leon.hwang@linux.dev> 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.
> 
> Agreed. I think the idea of verbose log is useful for bpf() syscall,
> given how complicated some of its conditions are. But it should be
> done more generically, ideally at syscall (or at least the entire BPF
> command) level, not for one particular kind of link.
> 

Cool!

But, how can we achieve it?

Thanks,
Leon

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

[Alexei Starovoitov]

On Tue, Feb 25, 2025 at 5:50 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>
>
>
> On 2025/2/25 03:41, Alexei Starovoitov wrote:
> > On Mon, Feb 24, 2025 at 7:34 AM Leon Hwang <leon.hwang@linux.dev> 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.
> >
>
> Got it.
>
> I'm planning to implement a restrict version of
> "bpf: make tracing program support multi-link"[0]. With log buffer, it
> will be helpful to report the reason for declining attaching, especially
> to report the tracee info that causes the attachment failure.
>
> [0]
> https://lore.kernel.org/bpf/20240311093526.1010158-1-dongmenglong.8@bytedance.com/

This is orthogonal.
"Some future feature may use it" is not an excuse to add code now.
Especially uapi.

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

[Andrii Nakryiko]

On Tue, Feb 25, 2025 at 5:59 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>
>
>
> On 2025/2/25 06:08, Andrii Nakryiko wrote:
> > On Mon, Feb 24, 2025 at 11:41 AM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >>
> >> On Mon, Feb 24, 2025 at 7:34 AM Leon Hwang <leon.hwang@linux.dev> 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.
> >
> > Agreed. I think the idea of verbose log is useful for bpf() syscall,
> > given how complicated some of its conditions are. But it should be
> > done more generically, ideally at syscall (or at least the entire BPF
> > command) level, not for one particular kind of link.
> >
>
> Cool!
>
> But, how can we achieve it?

There is no *elegant* way to do this, but I think we could retrofit
this as extra common bpf_attrs into existing bpf() syscall. Something
along the lines of:

struct bpf_common_attr {
    __u64 log_buf;
    __u32 log_size;
}

#define BPF_COMMON_ATTRS 0x80000000

static int __sys_bpf(enum bpf_cmd cmd, bpfptr_t uattr, unsigned int
size, bpfptr_t uattr_common, unsigned int size_common)
{
    if (cmd & BPF_COMMON_ATTRS) {
        cmd &= ~BPF_COMMON_ATTRS;
        /* read out bpf_common_attr from uattr_common */
    }
}


i.e., we add two extra arguments to bpf() syscall, but only look at
them if we have an extra flag set in cmd.

>
> Thanks,
> Leon
>

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

[Alexei Starovoitov]

On Tue, Feb 25, 2025 at 9:19 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> > But, how can we achieve it?
>
> There is no *elegant* way to do this, but I think we could retrofit
> this as extra common bpf_attrs into existing bpf() syscall. Something
> along the lines of:
>
> struct bpf_common_attr {
>     __u64 log_buf;
>     __u32 log_size;

other than missing log_level I like this approach.

> }
>
> #define BPF_COMMON_ATTRS 0x80000000

negative enum/int is a bit meh, can we use 64 instead?
In token we have:
BUILD_BUG_ON(__MAX_BPF_CMD >= 64);
and delegate_cmds mount option too.

Currently __MAX_BPF_CMD = 37
so we have some room.
BPF_COMMON_ATTRS (1 << 16) is fine too.
Just not the sign bit.

>
> static int __sys_bpf(enum bpf_cmd cmd, bpfptr_t uattr, unsigned int
> size, bpfptr_t uattr_common, unsigned int size_common)
> {
>     if (cmd & BPF_COMMON_ATTRS) {
>         cmd &= ~BPF_COMMON_ATTRS;
>         /* read out bpf_common_attr from uattr_common */
>     }
> }
>
>
> i.e., we add two extra arguments to bpf() syscall, but only look at
> them if we have an extra flag set in cmd.
>
> >
> > Thanks,
> > Leon
> >

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

[Leon Hwang]

On 2025/2/26 11:17, Alexei Starovoitov wrote:
> On Tue, Feb 25, 2025 at 9:19 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>>> But, how can we achieve it?
>>
>> There is no *elegant* way to do this, but I think we could retrofit
>> this as extra common bpf_attrs into existing bpf() syscall. Something
>> along the lines of:
>>
>> struct bpf_common_attr {
>>     __u64 log_buf;
>>     __u32 log_size;
> 
> other than missing log_level I like this approach.
> 
>> }
>>
>> #define BPF_COMMON_ATTRS 0x80000000
> 
> negative enum/int is a bit meh, can we use 64 instead?
> In token we have:
> BUILD_BUG_ON(__MAX_BPF_CMD >= 64);
> and delegate_cmds mount option too.
> 
> Currently __MAX_BPF_CMD = 37
> so we have some room.
> BPF_COMMON_ATTRS (1 << 16) is fine too.
> Just not the sign bit.
> 

Let me try this approach with BPF_COMMON_ATTRS (1 << 16).

Thanks,
Leon