[PATCH bpf-next/net v1 0/5] Fix bpf qdisc bugs and cleanup

[PATCH bpf-next/net v1 0/5] Fix bpf qdisc bugs and cleanup

[Amery Hung]

This patchset fixes the following bugs in bpf qdisc and cleanup the
selftest.

- A null-pointer dereference can happen in qdisc_watchdog_cancel() if the
  timer is not initialized when 1) .init is not defined by user so
  init prologue is not generated. 2) .init fails and qdisc_create()
  calls .destroy

- bpf qdisc fails to attach to mq/mqprio when being set as the default
  qdisc due to failed qdisc_lookup() in init prologue

Amery Hung (5):
  bpf: net_sched: Fix bpf qdisc init prologue when set as default qdisc
  selftests/bpf: Test setting and creating bpf qdisc as default qdisc
  bpf: net_sched: Make some Qdisc_ops ops mandatory
  selftests/bpf: selftests/bpf: Test attaching a bpf qdisc with
    incomplete operators
  selftests/bpf: Cleanup bpf qdisc selftests

 net/sched/bpf_qdisc.c                         | 24 ++++-
 .../selftests/bpf/prog_tests/bpf_qdisc.c      | 95 ++++++++++++++++++-
 .../selftests/bpf/progs/bpf_qdisc_common.h    |  6 --
 .../bpf/progs/bpf_qdisc_fail__incompl_ops.c   | 41 ++++++++
 .../selftests/bpf/progs/bpf_qdisc_fifo.c      |  6 ++
 .../selftests/bpf/progs/bpf_qdisc_fq.c        |  6 ++
 6 files changed, 164 insertions(+), 14 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_qdisc_fail__incompl_ops.c

-- 
2.47.1

[PATCH bpf-next/net v1 1/5] bpf: net_sched: Fix bpf qdisc init prologue when set as default qdisc

[Amery Hung]

Allow .init to proceed if qdisc_lookup() returns NULL as it only happens
when called by qdisc_create_dflt() in mq/mqprio_init and the parent qdisc
has not been added to qdisc_hash yet. In qdisc_create(), the caller,
__tc_modify_qdisc(), would have made sure the parent qdisc already exist.

In addition, call qdisc_watchdog_init() whether .init succeeds or not to
prevent null-pointer dereference. In qdisc_create() and
qdisc_create_dflt(), if .init fails, .destroy will be called. As a
result, the destroy epilogue could call qdisc_watchdog_cancel() with an
uninitialized timer, causing null-pointer deference in hrtimer_cancel().

Fixes: Fixes: c8240344956e ("bpf: net_sched: Support implementation of Qdisc_ops in bpf")
Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 net/sched/bpf_qdisc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
index 9f32b305636f..a8efc3ff2b7e 100644
--- a/net/sched/bpf_qdisc.c
+++ b/net/sched/bpf_qdisc.c
@@ -234,18 +234,20 @@ __bpf_kfunc int bpf_qdisc_init_prologue(struct Qdisc *sch,
 	struct net_device *dev = qdisc_dev(sch);
 	struct Qdisc *p;
 
+	qdisc_watchdog_init(&q->watchdog, sch);
+
 	if (sch->parent != TC_H_ROOT) {
+		/* If qdisc_lookup() returns NULL, it means .init is called by
+		 * qdisc_create_dflt() in mq/mqprio_init and the parent qdisc
+		 * has not been added to qdisc_hash yet.
+		 */
 		p = qdisc_lookup(dev, TC_H_MAJ(sch->parent));
-		if (!p)
-			return -ENOENT;
-
-		if (!(p->flags & TCQ_F_MQROOT)) {
+		if (p && !(p->flags & TCQ_F_MQROOT)) {
 			NL_SET_ERR_MSG(extack, "BPF qdisc only supported on root or mq");
 			return -EINVAL;
 		}
 	}
 
-	qdisc_watchdog_init(&q->watchdog, sch);
 	return 0;
 }
 
-- 
2.47.1

Re: [PATCH bpf-next/net v1 1/5] bpf: net_sched: Fix bpf qdisc init prologue when set as default qdisc

[Simon Horman]

On Thu, May 01, 2025 at 03:30:21PM -0700, Amery Hung wrote:
> Allow .init to proceed if qdisc_lookup() returns NULL as it only happens
> when called by qdisc_create_dflt() in mq/mqprio_init and the parent qdisc
> has not been added to qdisc_hash yet. In qdisc_create(), the caller,
> __tc_modify_qdisc(), would have made sure the parent qdisc already exist.
> 
> In addition, call qdisc_watchdog_init() whether .init succeeds or not to
> prevent null-pointer dereference. In qdisc_create() and
> qdisc_create_dflt(), if .init fails, .destroy will be called. As a
> result, the destroy epilogue could call qdisc_watchdog_cancel() with an
> uninitialized timer, causing null-pointer deference in hrtimer_cancel().
> 
> Fixes: Fixes: c8240344956e ("bpf: net_sched: Support implementation of Qdisc_ops in bpf")

nit: One "Fixes: " is enough.

> Signed-off-by: Amery Hung <ameryhung@gmail.com>

...

[PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and creating bpf qdisc as default qdisc

[Amery Hung]

First, test that bpf qdisc can be set as default qdisc. Then, attach
an mq qdisc to see if bpf qdisc can be successfully created and grafted.

The test is a sequential test as net.core.default_qdisc is global.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 .../selftests/bpf/prog_tests/bpf_qdisc.c      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
index c9a54177c84e..c954cc2ae64f 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
@@ -159,6 +159,79 @@ static void test_qdisc_attach_to_non_root(void)
 	bpf_qdisc_fifo__destroy(fifo_skel);
 }
 
+static int get_default_qdisc(char *qdisc_name)
+{
+	FILE *f;
+	int num;
+
+	f = fopen("/proc/sys/net/core/default_qdisc", "r");
+	if (!f)
+		return -errno;
+
+	num = fscanf(f, "%s", qdisc_name);
+	fclose(f);
+
+	return num == 1 ? 0 : -EFAULT;
+}
+
+static void test_default_qdisc_attach_to_mq(void)
+{
+	struct bpf_qdisc_fifo *fifo_skel;
+	char default_qdisc[IFNAMSIZ];
+	struct netns_obj *netns;
+	char tc_qdisc_show[64];
+	struct bpf_link *link;
+	char *str_ret;
+	FILE *tc;
+	int err;
+
+	fifo_skel = bpf_qdisc_fifo__open_and_load();
+	if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(fifo_skel->maps.fifo);
+	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
+		bpf_qdisc_fifo__destroy(fifo_skel);
+		return;
+	}
+
+	err = get_default_qdisc(default_qdisc);
+	if (!ASSERT_OK(err, "read sysctl net.core.default_qdisc"))
+		goto out;
+
+	err = write_sysctl("/proc/sys/net/core/default_qdisc", "bpf_fifo");
+	if (!ASSERT_OK(err, "write sysctl net.core.default_qdisc"))
+		goto out;
+
+	netns = netns_new("bpf_qdisc_ns", true);
+	if (!ASSERT_OK_PTR(netns, "netns_new"))
+		goto out;
+
+	SYS(out_restore_dflt_qdisc, "ip link add veth0 type veth peer veth1");
+	SYS(out_delete_netns, "tc qdisc add dev veth0 root handle 1: mq");
+
+	tc = popen("tc qdisc show dev veth0 parent 1:1", "r");
+	if (!ASSERT_OK_PTR(tc, "tc qdisc show dev veth0 parent 1:1"))
+		goto out_delete_netns;
+
+	str_ret = fgets(tc_qdisc_show, sizeof(tc_qdisc_show), tc);
+	if (!ASSERT_OK_PTR(str_ret, "tc qdisc show dev veth0 parent 1:1"))
+		goto out_delete_netns;
+
+	str_ret = strstr(tc_qdisc_show, "qdisc bpf_fifo");
+	if (!ASSERT_OK_PTR(str_ret, "check if bpf_fifo is created"))
+		goto out_delete_netns;
+
+	SYS(out_delete_netns, "tc qdisc delete dev veth0 root mq");
+out_delete_netns:
+	netns_free(netns);
+out_restore_dflt_qdisc:
+	write_sysctl("/proc/sys/net/core/default_qdisc", default_qdisc);
+out:
+	bpf_link__destroy(link);
+	bpf_qdisc_fifo__destroy(fifo_skel);
+}
+
 void test_bpf_qdisc(void)
 {
 	struct netns_obj *netns;
@@ -178,3 +251,8 @@ void test_bpf_qdisc(void)
 
 	netns_free(netns);
 }
+
+void serial_test_bpf_qdisc_default(void)
+{
+	test_default_qdisc_attach_to_mq();
+}
-- 
2.47.1

Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and creating bpf qdisc as default qdisc

[Martin KaFai Lau]

On 5/1/25 3:30 PM, Amery Hung wrote:
> First, test that bpf qdisc can be set as default qdisc. Then, attach
> an mq qdisc to see if bpf qdisc can be successfully created and grafted.
> 
> The test is a sequential test as net.core.default_qdisc is global.
> 
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---
>   .../selftests/bpf/prog_tests/bpf_qdisc.c      | 78 +++++++++++++++++++
>   1 file changed, 78 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> index c9a54177c84e..c954cc2ae64f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> @@ -159,6 +159,79 @@ static void test_qdisc_attach_to_non_root(void)
>   	bpf_qdisc_fifo__destroy(fifo_skel);
>   }
>   
> +static int get_default_qdisc(char *qdisc_name)
> +{
> +	FILE *f;
> +	int num;
> +
> +	f = fopen("/proc/sys/net/core/default_qdisc", "r");
> +	if (!f)
> +		return -errno;
> +
> +	num = fscanf(f, "%s", qdisc_name);
> +	fclose(f);
> +
> +	return num == 1 ? 0 : -EFAULT;
> +}
> +
> +static void test_default_qdisc_attach_to_mq(void)
> +{
> +	struct bpf_qdisc_fifo *fifo_skel;
> +	char default_qdisc[IFNAMSIZ];
> +	struct netns_obj *netns;
> +	char tc_qdisc_show[64];
> +	struct bpf_link *link;
> +	char *str_ret;
> +	FILE *tc;
> +	int err;
> +
> +	fifo_skel = bpf_qdisc_fifo__open_and_load();
> +	if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(fifo_skel->maps.fifo);

	fifo_skel->links.fifo = bpf_map__attach_struct_ops(....);

Then no need to bpf_link__destroy(link). bpf_qdisc_fifo__destroy() should do.

> +	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
> +		bpf_qdisc_fifo__destroy(fifo_skel);
> +		return;
> +	}
> +
> +	err = get_default_qdisc(default_qdisc);
> +	if (!ASSERT_OK(err, "read sysctl net.core.default_qdisc"))
> +		goto out;
> +
> +	err = write_sysctl("/proc/sys/net/core/default_qdisc", "bpf_fifo");
> +	if (!ASSERT_OK(err, "write sysctl net.core.default_qdisc"))
> +		goto out;
> +
> +	netns = netns_new("bpf_qdisc_ns", true);
> +	if (!ASSERT_OK_PTR(netns, "netns_new"))
> +		goto out;

This should be 'goto out_restore_dflt_qdisc'.

I would stay with minimum number of cleanup labels if possible. Initialize the 
variables that need to be cleaned up instead. There is no need to optimize each 
cleanup case for a test,

e.g. "struct netns_obj netns = NULL; char default_qdisc[IFNAMSIZ] = {};..."

> +
> +	SYS(out_restore_dflt_qdisc, "ip link add veth0 type veth peer veth1");
> +	SYS(out_delete_netns, "tc qdisc add dev veth0 root handle 1: mq");
> +
> +	tc = popen("tc qdisc show dev veth0 parent 1:1", "r");
> +	if (!ASSERT_OK_PTR(tc, "tc qdisc show dev veth0 parent 1:1"))
> +		goto out_delete_netns;
> +
> +	str_ret = fgets(tc_qdisc_show, sizeof(tc_qdisc_show), tc);
> +	if (!ASSERT_OK_PTR(str_ret, "tc qdisc show dev veth0 parent 1:1"))
> +		goto out_delete_netns;
> +
> +	str_ret = strstr(tc_qdisc_show, "qdisc bpf_fifo");
> +	if (!ASSERT_OK_PTR(str_ret, "check if bpf_fifo is created"))
> +		goto out_delete_netns;

Instead of pipe and grep, how about having the bpf_fifo_init bpf prog to set a 
global variable when called and then check the "fifo_skel->bss->init_called == 
true" here?

> +
> +	SYS(out_delete_netns, "tc qdisc delete dev veth0 root mq");
> +out_delete_netns:
> +	netns_free(netns);
> +out_restore_dflt_qdisc:
> +	write_sysctl("/proc/sys/net/core/default_qdisc", default_qdisc);
> +out:
> +	bpf_link__destroy(link);
> +	bpf_qdisc_fifo__destroy(fifo_skel);
> +}
> +
>   void test_bpf_qdisc(void)
>   {
>   	struct netns_obj *netns;
> @@ -178,3 +251,8 @@ void test_bpf_qdisc(void)
>   
>   	netns_free(netns);
>   }
> +
> +void serial_test_bpf_qdisc_default(void)
> +{
> +	test_default_qdisc_attach_to_mq();
> +}

Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and creating bpf qdisc as default qdisc

[Amery Hung]

On Thu, May 1, 2025 at 4:58 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 5/1/25 3:30 PM, Amery Hung wrote:
> > First, test that bpf qdisc can be set as default qdisc. Then, attach
> > an mq qdisc to see if bpf qdisc can be successfully created and grafted.
> >
> > The test is a sequential test as net.core.default_qdisc is global.
> >
> > Signed-off-by: Amery Hung <ameryhung@gmail.com>
> > ---
> >   .../selftests/bpf/prog_tests/bpf_qdisc.c      | 78 +++++++++++++++++++
> >   1 file changed, 78 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> > index c9a54177c84e..c954cc2ae64f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> > @@ -159,6 +159,79 @@ static void test_qdisc_attach_to_non_root(void)
> >       bpf_qdisc_fifo__destroy(fifo_skel);
> >   }
> >
> > +static int get_default_qdisc(char *qdisc_name)
> > +{
> > +     FILE *f;
> > +     int num;
> > +
> > +     f = fopen("/proc/sys/net/core/default_qdisc", "r");
> > +     if (!f)
> > +             return -errno;
> > +
> > +     num = fscanf(f, "%s", qdisc_name);
> > +     fclose(f);
> > +
> > +     return num == 1 ? 0 : -EFAULT;
> > +}
> > +
> > +static void test_default_qdisc_attach_to_mq(void)
> > +{
> > +     struct bpf_qdisc_fifo *fifo_skel;
> > +     char default_qdisc[IFNAMSIZ];
> > +     struct netns_obj *netns;
> > +     char tc_qdisc_show[64];
> > +     struct bpf_link *link;
> > +     char *str_ret;
> > +     FILE *tc;
> > +     int err;
> > +
> > +     fifo_skel = bpf_qdisc_fifo__open_and_load();
> > +     if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))
> > +             return;
> > +
> > +     link = bpf_map__attach_struct_ops(fifo_skel->maps.fifo);
>
>         fifo_skel->links.fifo = bpf_map__attach_struct_ops(....);
>
> Then no need to bpf_link__destroy(link). bpf_qdisc_fifo__destroy() should do.
>

I see. I assume it will also be okay to set autoattach and call
bpf_qdisc_fifo__attach()?

> > +     if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
> > +             bpf_qdisc_fifo__destroy(fifo_skel);
> > +             return;
> > +     }
> > +
> > +     err = get_default_qdisc(default_qdisc);
> > +     if (!ASSERT_OK(err, "read sysctl net.core.default_qdisc"))
> > +             goto out;
> > +
> > +     err = write_sysctl("/proc/sys/net/core/default_qdisc", "bpf_fifo");
> > +     if (!ASSERT_OK(err, "write sysctl net.core.default_qdisc"))
> > +             goto out;
> > +
> > +     netns = netns_new("bpf_qdisc_ns", true);
> > +     if (!ASSERT_OK_PTR(netns, "netns_new"))
> > +             goto out;
>
> This should be 'goto out_restore_dflt_qdisc'.
>
> I would stay with minimum number of cleanup labels if possible. Initialize the
> variables that need to be cleaned up instead. There is no need to optimize each
> cleanup case for a test,
>
> e.g. "struct netns_obj netns = NULL; char default_qdisc[IFNAMSIZ] = {};..."
>

I will simplify the cleanup as you suggested.

> > +
> > +     SYS(out_restore_dflt_qdisc, "ip link add veth0 type veth peer veth1");
> > +     SYS(out_delete_netns, "tc qdisc add dev veth0 root handle 1: mq");
> > +
> > +     tc = popen("tc qdisc show dev veth0 parent 1:1", "r");
> > +     if (!ASSERT_OK_PTR(tc, "tc qdisc show dev veth0 parent 1:1"))
> > +             goto out_delete_netns;
> > +
> > +     str_ret = fgets(tc_qdisc_show, sizeof(tc_qdisc_show), tc);
> > +     if (!ASSERT_OK_PTR(str_ret, "tc qdisc show dev veth0 parent 1:1"))
> > +             goto out_delete_netns;
> > +
> > +     str_ret = strstr(tc_qdisc_show, "qdisc bpf_fifo");
> > +     if (!ASSERT_OK_PTR(str_ret, "check if bpf_fifo is created"))
> > +             goto out_delete_netns;
>
> Instead of pipe and grep, how about having the bpf_fifo_init bpf prog to set a
> global variable when called and then check the "fifo_skel->bss->init_called ==
> true" here?
>

That simplifies things quite a bit. I will use a global variable to
check if the qdisc is being created.

> > +
> > +     SYS(out_delete_netns, "tc qdisc delete dev veth0 root mq");
> > +out_delete_netns:
> > +     netns_free(netns);
> > +out_restore_dflt_qdisc:
> > +     write_sysctl("/proc/sys/net/core/default_qdisc", default_qdisc);
> > +out:
> > +     bpf_link__destroy(link);
> > +     bpf_qdisc_fifo__destroy(fifo_skel);
> > +}
> > +
> >   void test_bpf_qdisc(void)
> >   {
> >       struct netns_obj *netns;
> > @@ -178,3 +251,8 @@ void test_bpf_qdisc(void)
> >
> >       netns_free(netns);
> >   }
> > +
> > +void serial_test_bpf_qdisc_default(void)
> > +{
> > +     test_default_qdisc_attach_to_mq();
> > +}
>

Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and creating bpf qdisc as default qdisc

[Martin KaFai Lau]

On 5/2/25 10:52 AM, Amery Hung wrote:
>>> +static void test_default_qdisc_attach_to_mq(void)
>>> +{
>>> +     struct bpf_qdisc_fifo *fifo_skel;
>>> +     char default_qdisc[IFNAMSIZ];
>>> +     struct netns_obj *netns;
>>> +     char tc_qdisc_show[64];
>>> +     struct bpf_link *link;
>>> +     char *str_ret;
>>> +     FILE *tc;
>>> +     int err;
>>> +
>>> +     fifo_skel = bpf_qdisc_fifo__open_and_load();
>>> +     if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))
>>> +             return;
>>> +
>>> +     link = bpf_map__attach_struct_ops(fifo_skel->maps.fifo);
>>
>>          fifo_skel->links.fifo = bpf_map__attach_struct_ops(....);
>>
>> Then no need to bpf_link__destroy(link). bpf_qdisc_fifo__destroy() should do.
>>
> 
> I see. I assume it will also be okay to set autoattach and call
> bpf_qdisc_fifo__attach()?

Good point. bpf_qdisc_fifo__attach() will be even simpler. I thought the 
autoattach is true by default. Please check.

Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and creating bpf qdisc as default qdisc

[Amery Hung]

On Fri, May 2, 2025 at 12:27 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 5/2/25 10:52 AM, Amery Hung wrote:
> >>> +static void test_default_qdisc_attach_to_mq(void)
> >>> +{
> >>> +     struct bpf_qdisc_fifo *fifo_skel;
> >>> +     char default_qdisc[IFNAMSIZ];
> >>> +     struct netns_obj *netns;
> >>> +     char tc_qdisc_show[64];
> >>> +     struct bpf_link *link;
> >>> +     char *str_ret;
> >>> +     FILE *tc;
> >>> +     int err;
> >>> +
> >>> +     fifo_skel = bpf_qdisc_fifo__open_and_load();
> >>> +     if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))
> >>> +             return;
> >>> +
> >>> +     link = bpf_map__attach_struct_ops(fifo_skel->maps.fifo);
> >>
> >>          fifo_skel->links.fifo = bpf_map__attach_struct_ops(....);
> >>
> >> Then no need to bpf_link__destroy(link). bpf_qdisc_fifo__destroy() should do.
> >>
> >
> > I see. I assume it will also be okay to set autoattach and call
> > bpf_qdisc_fifo__attach()?
>
> Good point. bpf_qdisc_fifo__attach() will be even simpler. I thought the
> autoattach is true by default. Please check.
>

You are right. It is true by default. I will also clean up other qdisc
subtests in this way.

[PATCH bpf-next/net v1 3/5] bpf: net_sched: Make some Qdisc_ops ops mandatory

[Amery Hung]

The patch makes all currently supported Qdisc_ops (i.e., .enqueue,
.dequeue, .init, .reser, and .destroy) mandatory.

Make .init, .reset and .destroy mandatory as bpf qdisc relies on prologue
and epilogue to check attach points and correctly initialize/cleanup
resources. The prologue/epilogue will only be generated for an struct_ops
operator only if users implement the operator.

Make .enqueue and .dequeue mandatory as bpf qdisc infra does not provide
a default data path.

Fixes: Fixes: c8240344956e ("bpf: net_sched: Support implementation of Qdisc_ops in bpf")
Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 net/sched/bpf_qdisc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
index a8efc3ff2b7e..7ea8b54b2ab1 100644
--- a/net/sched/bpf_qdisc.c
+++ b/net/sched/bpf_qdisc.c
@@ -395,6 +395,17 @@ static void bpf_qdisc_unreg(void *kdata, struct bpf_link *link)
 	return unregister_qdisc(kdata);
 }
 
+static int bpf_qdisc_validate(void *kdata)
+{
+	struct Qdisc_ops *ops = (struct Qdisc_ops *)kdata;
+
+	if (!ops->enqueue || !ops->dequeue || !ops->init ||
+	    !ops->reset || !ops->destroy)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int Qdisc_ops__enqueue(struct sk_buff *skb__ref, struct Qdisc *sch,
 			      struct sk_buff **to_free)
 {
@@ -432,6 +443,7 @@ static struct bpf_struct_ops bpf_Qdisc_ops = {
 	.verifier_ops = &bpf_qdisc_verifier_ops,
 	.reg = bpf_qdisc_reg,
 	.unreg = bpf_qdisc_unreg,
+	.validate = bpf_qdisc_validate,
 	.init_member = bpf_qdisc_init_member,
 	.init = bpf_qdisc_init,
 	.name = "Qdisc_ops",
-- 
2.47.1

Re: [PATCH bpf-next/net v1 3/5] bpf: net_sched: Make some Qdisc_ops ops mandatory

[Martin KaFai Lau]

On 5/1/25 3:30 PM, Amery Hung wrote:
> The patch makes all currently supported Qdisc_ops (i.e., .enqueue,
> .dequeue, .init, .reser, and .destroy) mandatory.

s/reser/reset/.

> 
> Make .init, .reset and .destroy mandatory as bpf qdisc relies on prologue
> and epilogue to check attach points and correctly initialize/cleanup
> resources. The prologue/epilogue will only be generated for an struct_ops
> operator only if users implement the operator.
> 
> Make .enqueue and .dequeue mandatory as bpf qdisc infra does not provide
> a default data path.
> 
> Fixes: Fixes: c8240344956e ("bpf: net_sched: Support implementation of Qdisc_ops in bpf")
> Signed-off-by: Amery Hung <ameryhung@gmail.com>
> ---
>   net/sched/bpf_qdisc.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
> index a8efc3ff2b7e..7ea8b54b2ab1 100644
> --- a/net/sched/bpf_qdisc.c
> +++ b/net/sched/bpf_qdisc.c
> @@ -395,6 +395,17 @@ static void bpf_qdisc_unreg(void *kdata, struct bpf_link *link)
>   	return unregister_qdisc(kdata);
>   }
>   
> +static int bpf_qdisc_validate(void *kdata)
> +{
> +	struct Qdisc_ops *ops = (struct Qdisc_ops *)kdata;
> +
> +	if (!ops->enqueue || !ops->dequeue || !ops->init ||
> +	    !ops->reset || !ops->destroy)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   static int Qdisc_ops__enqueue(struct sk_buff *skb__ref, struct Qdisc *sch,
>   			      struct sk_buff **to_free)
>   {
> @@ -432,6 +443,7 @@ static struct bpf_struct_ops bpf_Qdisc_ops = {
>   	.verifier_ops = &bpf_qdisc_verifier_ops,
>   	.reg = bpf_qdisc_reg,
>   	.unreg = bpf_qdisc_unreg,
> +	.validate = bpf_qdisc_validate,
>   	.init_member = bpf_qdisc_init_member,
>   	.init = bpf_qdisc_init,
>   	.name = "Qdisc_ops",

[PATCH bpf-next/net v1 4/5] selftests/bpf: selftests/bpf: Test attaching a bpf qdisc with incomplete operators

[Amery Hung]

Implement .destroy in bpf_fq and bpf_fifo as it is now mandatory.

Test attaching a bpf qdisc with a missing operator .init. This is not
allowed as bpf qdisc qdisc_watchdog_cancel() could have been called with
an uninitialized timer.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 .../selftests/bpf/prog_tests/bpf_qdisc.c      | 19 +++++++++
 .../bpf/progs/bpf_qdisc_fail__incompl_ops.c   | 41 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_qdisc_fifo.c      |  6 +++
 .../selftests/bpf/progs/bpf_qdisc_fq.c        |  6 +++
 4 files changed, 72 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_qdisc_fail__incompl_ops.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
index c954cc2ae64f..8afaf71cfadd 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
@@ -7,6 +7,7 @@
 #include "network_helpers.h"
 #include "bpf_qdisc_fifo.skel.h"
 #include "bpf_qdisc_fq.skel.h"
+#include "bpf_qdisc_fail__incompl_ops.skel.h"
 
 #define LO_IFINDEX 1
 
@@ -159,6 +160,22 @@ static void test_qdisc_attach_to_non_root(void)
 	bpf_qdisc_fifo__destroy(fifo_skel);
 }
 
+static void test_incompl_ops(void)
+{
+	struct bpf_qdisc_fail__incompl_ops *skel;
+	struct bpf_link *link;
+
+	skel = bpf_qdisc_fail__incompl_ops__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "bpf_qdisc_fifo__open_and_load"))
+		return;
+
+	link = bpf_map__attach_struct_ops(skel->maps.test);
+	if (!ASSERT_ERR_PTR(link, "bpf_map__attach_struct_ops"))
+		bpf_link__destroy(link);
+
+	bpf_qdisc_fail__incompl_ops__destroy(skel);
+}
+
 static int get_default_qdisc(char *qdisc_name)
 {
 	FILE *f;
@@ -248,6 +265,8 @@ void test_bpf_qdisc(void)
 		test_qdisc_attach_to_mq();
 	if (test__start_subtest("attach to non root"))
 		test_qdisc_attach_to_non_root();
+	if (test__start_subtest("incompl_ops"))
+		test_incompl_ops();
 
 	netns_free(netns);
 }
diff --git a/tools/testing/selftests/bpf/progs/bpf_qdisc_fail__incompl_ops.c b/tools/testing/selftests/bpf/progs/bpf_qdisc_fail__incompl_ops.c
new file mode 100644
index 000000000000..49d790d2ea03
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_qdisc_fail__incompl_ops.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <vmlinux.h>
+#include "bpf_experimental.h"
+#include "bpf_qdisc_common.h"
+
+char _license[] SEC("license") = "GPL";
+
+SEC("struct_ops/bpf_qdisc_test_enqueue")
+int BPF_PROG(bpf_qdisc_test_enqueue, struct sk_buff *skb, struct Qdisc *sch,
+	     struct bpf_sk_buff_ptr *to_free)
+{
+	bpf_qdisc_skb_drop(skb, to_free);
+	return NET_XMIT_DROP;
+}
+
+SEC("struct_ops/bpf_qdisc_test_dequeue")
+struct sk_buff *BPF_PROG(bpf_qdisc_test_dequeue, struct Qdisc *sch)
+{
+	return NULL;
+}
+
+SEC("struct_ops/bpf_qdisc_test_reset")
+void BPF_PROG(bpf_qdisc_test_reset, struct Qdisc *sch)
+{
+}
+
+SEC("struct_ops/bpf_qdisc_test_destroy")
+void BPF_PROG(bpf_qdisc_test_destroy, struct Qdisc *sch)
+{
+}
+
+SEC(".struct_ops")
+struct Qdisc_ops test = {
+	.enqueue   = (void *)bpf_qdisc_test_enqueue,
+	.dequeue   = (void *)bpf_qdisc_test_dequeue,
+	.reset     = (void *)bpf_qdisc_test_reset,
+	.destroy   = (void *)bpf_qdisc_test_destroy,
+	.id        = "bpf_qdisc_test",
+};
+
diff --git a/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c b/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c
index 0c7cfb82dae1..6628d2872820 100644
--- a/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c
+++ b/tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c
@@ -106,12 +106,18 @@ void BPF_PROG(bpf_fifo_reset, struct Qdisc *sch)
 	sch->q.qlen = 0;
 }
 
+SEC("struct_ops/bpf_fifo_destroy")
+void BPF_PROG(bpf_fifo_destroy, struct Qdisc *sch)
+{
+}
+
 SEC(".struct_ops")
 struct Qdisc_ops fifo = {
 	.enqueue   = (void *)bpf_fifo_enqueue,
 	.dequeue   = (void *)bpf_fifo_dequeue,
 	.init      = (void *)bpf_fifo_init,
 	.reset     = (void *)bpf_fifo_reset,
+	.destroy   = (void *)bpf_fifo_destroy,
 	.id        = "bpf_fifo",
 };
 
diff --git a/tools/testing/selftests/bpf/progs/bpf_qdisc_fq.c b/tools/testing/selftests/bpf/progs/bpf_qdisc_fq.c
index 7c110a156224..72c9f4aefbcf 100644
--- a/tools/testing/selftests/bpf/progs/bpf_qdisc_fq.c
+++ b/tools/testing/selftests/bpf/progs/bpf_qdisc_fq.c
@@ -740,11 +740,17 @@ int BPF_PROG(bpf_fq_init, struct Qdisc *sch, struct nlattr *opt,
 	return 0;
 }
 
+SEC("struct_ops/bpf_fq_destroy")
+void BPF_PROG(bpf_fq_destroy, struct Qdisc *sch)
+{
+}
+
 SEC(".struct_ops")
 struct Qdisc_ops fq = {
 	.enqueue   = (void *)bpf_fq_enqueue,
 	.dequeue   = (void *)bpf_fq_dequeue,
 	.reset     = (void *)bpf_fq_reset,
 	.init      = (void *)bpf_fq_init,
+	.destroy   = (void *)bpf_fq_destroy,
 	.id        = "bpf_fq",
 };
-- 
2.47.1

[PATCH bpf-next/net v1 5/5] selftests/bpf: Cleanup bpf qdisc selftests

[Amery Hung]

Some cleanups:
- Remove unnecessary kfuncs declaration
- Use _ns in the test name to run tests in a separate net namespace

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c   | 10 +---------
 tools/testing/selftests/bpf/progs/bpf_qdisc_common.h |  6 ------
 2 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
index 8afaf71cfadd..2c10816c2d9e 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
@@ -249,14 +249,8 @@ static void test_default_qdisc_attach_to_mq(void)
 	bpf_qdisc_fifo__destroy(fifo_skel);
 }
 
-void test_bpf_qdisc(void)
+void test_ns_bpf_qdisc(void)
 {
-	struct netns_obj *netns;
-
-	netns = netns_new("bpf_qdisc_ns", true);
-	if (!ASSERT_OK_PTR(netns, "netns_new"))
-		return;
-
 	if (test__start_subtest("fifo"))
 		test_fifo();
 	if (test__start_subtest("fq"))
@@ -267,8 +261,6 @@ void test_bpf_qdisc(void)
 		test_qdisc_attach_to_non_root();
 	if (test__start_subtest("incompl_ops"))
 		test_incompl_ops();
-
-	netns_free(netns);
 }
 
 void serial_test_bpf_qdisc_default(void)
diff --git a/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h b/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
index 65a2c561c0bb..c495da1c43db 100644
--- a/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
+++ b/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
@@ -12,12 +12,6 @@
 
 #define private(name) SEC(".data." #name) __hidden __attribute__((aligned(8)))
 
-u32 bpf_skb_get_hash(struct sk_buff *p) __ksym;
-void bpf_kfree_skb(struct sk_buff *p) __ksym;
-void bpf_qdisc_skb_drop(struct sk_buff *p, struct bpf_sk_buff_ptr *to_free) __ksym;
-void bpf_qdisc_watchdog_schedule(struct Qdisc *sch, u64 expire, u64 delta_ns) __ksym;
-void bpf_qdisc_bstats_update(struct Qdisc *sch, const struct sk_buff *skb) __ksym;
-
 static struct qdisc_skb_cb *qdisc_skb_cb(const struct sk_buff *skb)
 {
 	return (struct qdisc_skb_cb *)skb->cb;
-- 
2.47.1

Re: [PATCH bpf-next/net v1 5/5] selftests/bpf: Cleanup bpf qdisc selftests

[Martin KaFai Lau]

On 5/1/25 3:30 PM, Amery Hung wrote:
> diff --git a/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h b/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
> index 65a2c561c0bb..c495da1c43db 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h

There is a recent change landed in this file.

Overall the set makes sense. Thanks.

Re: [PATCH bpf-next/net v1 5/5] selftests/bpf: Cleanup bpf qdisc selftests

[Amery Hung]

On Thu, May 1, 2025 at 5:22 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 5/1/25 3:30 PM, Amery Hung wrote:
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h b/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
> > index 65a2c561c0bb..c495da1c43db 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
> > +++ b/tools/testing/selftests/bpf/progs/bpf_qdisc_common.h
>
> There is a recent change landed in this file.
>
> Overall the set makes sense. Thanks.

Thanks for the feedback. I will rebase and resend.

Thanks,
Amery