[PATCH bpf-next v4 0/2] bpf, xdp: Add tracepoint to xdp attaching failure

[PATCH bpf-next v4 0/2] bpf, xdp: Add tracepoint to xdp attaching failure

[Leon Hwang]

This series introduces a new tracepoint in bpf_xdp_link_attach(). By
this tracepoint, error message will be captured when error happens in
dev_xdp_attach(), e.g. invalid attaching flags.

v3 -> v4:
* Fix selftest-crashed issue.

Leon Hwang (2):
  bpf, xdp: Add tracepoint to xdp attaching failure
  selftests/bpf: Add testcase for xdp attaching failure tracepoint

 include/trace/events/xdp.h                    | 17 +++++
 net/core/dev.c                                |  5 +-
 .../selftests/bpf/prog_tests/xdp_attach.c     | 65 +++++++++++++++++++
 .../bpf/progs/test_xdp_attach_fail.c          | 54 +++++++++++++++
 4 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_attach_fail.c


base-commit: a33d978500acd8fb67efac9773ba0a8502c1ff06
-- 
2.41.0

[PATCH bpf-next v4 1/2] bpf, xdp: Add tracepoint to xdp attaching failure

[Leon Hwang]

When error happens in dev_xdp_attach(), it should have a way to tell
users the error message like the netlink approach.

To avoid breaking uapi, adding a tracepoint in bpf_xdp_link_attach() is
an appropriate way to notify users the error message.

Hence, bpf libraries are able to retrieve the error message by this
tracepoint, and then report the error message to users.

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 include/trace/events/xdp.h | 17 +++++++++++++++++
 net/core/dev.c             |  5 ++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index c40fc97f94171..cd89f1d5ce7b8 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -404,6 +404,23 @@ TRACE_EVENT(mem_return_failed,
 	)
 );
 
+TRACE_EVENT(bpf_xdp_link_attach_failed,
+
+	TP_PROTO(const char *msg),
+
+	TP_ARGS(msg),
+
+	TP_STRUCT__entry(
+		__string(msg, msg)
+	),
+
+	TP_fast_assign(
+		__assign_str(msg, msg);
+	),
+
+	TP_printk("errmsg=%s", __get_str(msg))
+);
+
 #endif /* _TRACE_XDP_H */
 
 #include <trace/define_trace.h>
diff --git a/net/core/dev.c b/net/core/dev.c
index 8e7d0cb540cdb..49bed890f807e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -133,6 +133,7 @@
 #include <trace/events/net.h>
 #include <trace/events/skb.h>
 #include <trace/events/qdisc.h>
+#include <trace/events/xdp.h>
 #include <linux/inetdevice.h>
 #include <linux/cpu_rmap.h>
 #include <linux/static_key.h>
@@ -9472,6 +9473,7 @@ int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 	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();
@@ -9497,12 +9499,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);
+		trace_bpf_xdp_link_attach_failed(extack._msg);
 		goto out_put_dev;
 	}
 
-- 
2.41.0

Re: [PATCH bpf-next v4 1/2] bpf, xdp: Add tracepoint to xdp attaching failure

[Jakub Kicinski]

On Sun, 30 Jul 2023 19:49:50 +0800 Leon Hwang wrote:
> @@ -9472,6 +9473,7 @@ int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  	struct bpf_link_primer link_primer;
>  	struct bpf_xdp_link *link;
>  	struct net_device *dev;
> +	struct netlink_ext_ack extack;

This is not initialized. Also, while fixing that, move it up 
to maintain the line order by decreasing line length.

>  	int err, fd;
>  
>  	rtnl_lock();
> @@ -9497,12 +9499,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);
> +		trace_bpf_xdp_link_attach_failed(extack._msg);

Re: [PATCH bpf-next v4 1/2] bpf, xdp: Add tracepoint to xdp attaching failure

[Leon Hwang]

On 1/8/23 10:43, Jakub Kicinski wrote:
> On Sun, 30 Jul 2023 19:49:50 +0800 Leon Hwang wrote:
>> @@ -9472,6 +9473,7 @@ int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>>  	struct bpf_link_primer link_primer;
>>  	struct bpf_xdp_link *link;
>>  	struct net_device *dev;
>> +	struct netlink_ext_ack extack;
> 
> This is not initialized. Also, while fixing that, move it up 
> to maintain the line order by decreasing line length.
> 

Thank you for your reviewing.

I'll fix it by initializing extack and moving the line to its appropriate position.

Thanks,
Leon

>>  	int err, fd;
>>  
>>  	rtnl_lock();
>> @@ -9497,12 +9499,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);
>> +		trace_bpf_xdp_link_attach_failed(extack._msg);

[PATCH bpf-next v4 2/2] selftests/bpf: Add testcase for xdp attaching failure tracepoint

[Leon Hwang]

Add a test case for the tracepoint of xdp attaching failure by bpf
tracepoint when attach XDP to a device with invalid flags option.

The bpf tracepoint retrieves error message from the tracepoint, and
then put the error message to a perf buffer. The testing code receives
error message from perf buffer, and then ASSERT "Invalid XDP flags for
BPF link attachment".

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
 .../selftests/bpf/prog_tests/xdp_attach.c     | 65 +++++++++++++++++++
 .../bpf/progs/test_xdp_attach_fail.c          | 54 +++++++++++++++
 2 files changed, 119 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_attach_fail.c

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
index fa3cac5488f5d..8c1cde74e9cd6 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <test_progs.h>
+#include "test_xdp_attach_fail.skel.h"
 
 #define IFINDEX_LO 1
 #define XDP_FLAGS_REPLACE		(1U << 4)
@@ -85,10 +86,74 @@ static void test_xdp_attach(const char *file)
 	bpf_object__close(obj1);
 }
 
+#define ERRMSG_LEN 64
+
+struct xdp_errmsg {
+	char msg[ERRMSG_LEN];
+};
+
+static void on_xdp_errmsg(void *ctx, int cpu, void *data, __u32 size)
+{
+	struct xdp_errmsg *ctx_errmg = ctx, *tp_errmsg = data;
+
+	memcpy(&ctx_errmg->msg, &tp_errmsg->msg, ERRMSG_LEN);
+}
+
+static const char tgt_errmsg[] = "Invalid XDP flags for BPF link attachment";
+
+static void test_xdp_attach_fail(const char *file)
+{
+	int err, fd_xdp;
+	struct bpf_object *obj = NULL;
+	struct test_xdp_attach_fail *skel = NULL;
+	struct perf_buffer *pb = NULL;
+	struct xdp_errmsg errmsg = {};
+
+	LIBBPF_OPTS(bpf_link_create_opts, opts);
+
+	skel = test_xdp_attach_fail__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_xdp_attach_fail__open_and_load"))
+		goto out_close;
+
+	err = test_xdp_attach_fail__attach(skel);
+	if (!ASSERT_EQ(err, 0, "test_xdp_attach_fail__attach"))
+		goto out_close;
+
+	/* set up perf buffer */
+	pb = perf_buffer__new(bpf_map__fd(skel->maps.xdp_errmsg_pb), 1,
+			      on_xdp_errmsg, NULL, &errmsg, NULL);
+	if (!ASSERT_OK_PTR(pb, "perf_buffer__new"))
+		goto out_close;
+
+	err = bpf_prog_test_load(file, BPF_PROG_TYPE_XDP, &obj, &fd_xdp);
+	if (!ASSERT_EQ(err, 0, "bpf_prog_test_load"))
+		goto out_close;
+
+	opts.flags = 0xFF; // invalid flags to fail to attach XDP prog
+	err = bpf_link_create(fd_xdp, IFINDEX_LO, BPF_XDP, &opts);
+	if (!ASSERT_EQ(err, -EINVAL, "bpf_link_create"))
+		goto out_close;
+
+	/* read perf buffer */
+	err = perf_buffer__poll(pb, 100);
+	if (!ASSERT_GT(err, -1, "perf_buffer__poll"))
+		goto out_close;
+
+	ASSERT_STRNEQ((const char *) errmsg.msg, tgt_errmsg,
+		      42 /* strlen(tgt_errmsg) */, "check error message");
+
+out_close:
+	perf_buffer__free(pb);
+	bpf_object__close(obj);
+	test_xdp_attach_fail__destroy(skel);
+}
+
 void serial_test_xdp_attach(void)
 {
 	if (test__start_subtest("xdp_attach"))
 		test_xdp_attach("./test_xdp.bpf.o");
 	if (test__start_subtest("xdp_attach_dynptr"))
 		test_xdp_attach("./test_xdp_dynptr.bpf.o");
+	if (test__start_subtest("xdp_attach_failed"))
+		test_xdp_attach_fail("./xdp_dummy.bpf.o");
 }
diff --git a/tools/testing/selftests/bpf/progs/test_xdp_attach_fail.c b/tools/testing/selftests/bpf/progs/test_xdp_attach_fail.c
new file mode 100644
index 0000000000000..d7149bbd95f75
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_xdp_attach_fail.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Leon Hwang */
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+#define ERRMSG_LEN 64
+
+struct xdp_errmsg {
+	char msg[ERRMSG_LEN];
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__type(key, int);
+	__type(value, int);
+} xdp_errmsg_pb SEC(".maps");
+
+struct xdp_attach_error_ctx {
+	unsigned long unused;
+
+	/*
+	 * bpf does not support tracepoint __data_loc directly.
+	 *
+	 * Actually, this field is a 32 bit integer whose value encodes
+	 * information on where to find the actual data. The first 2 bytes is
+	 * the size of the data. The last 2 bytes is the offset from the start
+	 * of the tracepoint struct where the data begins.
+	 * -- https://github.com/iovisor/bpftrace/pull/1542
+	 */
+	__u32 msg; // __data_loc char[] msg;
+};
+
+/*
+ * Catch the error message at the tracepoint.
+ */
+
+SEC("tp/xdp/bpf_xdp_link_attach_failed")
+int tp__xdp__bpf_xdp_link_attach_failed(struct xdp_attach_error_ctx *ctx)
+{
+	struct xdp_errmsg errmsg;
+	char *msg = (void *)(__u64) ((void *) ctx + (__u16) ctx->msg);
+
+	bpf_probe_read_kernel_str(&errmsg.msg, ERRMSG_LEN, msg);
+	bpf_perf_event_output(ctx, &xdp_errmsg_pb, BPF_F_CURRENT_CPU, &errmsg,
+			      ERRMSG_LEN);
+	return 0;
+}
+
+/*
+ * Reuse the XDP program in xdp_dummy.c.
+ */
+
+char LICENSE[] SEC("license") = "GPL";
-- 
2.41.0

Re: [PATCH bpf-next v4 0/2] bpf, xdp: Add tracepoint to xdp attaching failure

[Manjusaka]

This patch is very important to help us to debug the xdp program. At
the same time, we can make some monitoring tools to observe the kernel
status by using this trace event

李者璈 & Zheaoli

Email: lizheao940510@gmail.com
Github: https://github.com/Zheaoli


Leon Hwang <hffilwlqm@gmail.com> 于2023年7月30日周日 19:50写道：
>
> This series introduces a new tracepoint in bpf_xdp_link_attach(). By
> this tracepoint, error message will be captured when error happens in
> dev_xdp_attach(), e.g. invalid attaching flags.
>
> v3 -> v4:
> * Fix selftest-crashed issue.
>
> Leon Hwang (2):
>   bpf, xdp: Add tracepoint to xdp attaching failure
>   selftests/bpf: Add testcase for xdp attaching failure tracepoint
>
>  include/trace/events/xdp.h                    | 17 +++++
>  net/core/dev.c                                |  5 +-
>  .../selftests/bpf/prog_tests/xdp_attach.c     | 65 +++++++++++++++++++
>  .../bpf/progs/test_xdp_attach_fail.c          | 54 +++++++++++++++
>  4 files changed, 140 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_attach_fail.c
>
>
> base-commit: a33d978500acd8fb67efac9773ba0a8502c1ff06
> --
> 2.41.0
>
>

Re: [PATCH bpf-next v4 0/2] bpf, xdp: Add tracepoint to xdp attaching failure

[Leon Hwang]

On 2023/7/30 21:49, Manjusaka wrote:
> This patch is very important to help us to debug the xdp program. At
> the same time, we can make some monitoring tools to observe the kernel
> status by using this trace event
> 
> 李者璈 & Zheaoli
> 
> Email: lizheao940510@gmail.com
> Github: https://github.com/Zheaoli
> 

Thank you for your feedback.

I'm glad that it's helpful for you.

Thanks,
Leon