Commit 30190f82 authored by Martin KaFai Lau's avatar Martin KaFai Lau
Browse files

Merge branch 'fix-bpf-qdisc-bugs-and-clean-up' 

Amery Hung says:

====================
Fix bpf qdisc bugs and clean up

This patchset fixes the following bugs in bpf qdisc and clean up 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

v2
- Rebase to bpf-next/net
- Fix erroneous commit messages
- Fix and simplify selftests cleanup
  v1: https://lore.kernel.org/bpf/20250501223025.569020-1-ameryhung@gmail.com/
====================

Link: https://patch.msgid.link/20250502201624.3663079-1-ameryhung@gmail.com


Signed-off-by: default avatarMartin KaFai Lau <martin.lau@kernel.org>
parents 1b1f563a 2f9838e2

net/sched/bpf_qdisc.c

+19 −5
Original line number Diff line number Diff line
@@ -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;

}
@@ -393,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)

{
@@ -430,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",

tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c

+85 −34
Original line number Diff line number Diff line
@@ -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
@@ -49,42 +50,32 @@ static void do_test(char *qdisc) 
static void test_fifo(void)

{

	struct bpf_qdisc_fifo *fifo_skel;

	struct bpf_link *link;



	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;

	}

	if (!ASSERT_OK(bpf_qdisc_fifo__attach(fifo_skel), "bpf_qdisc_fifo__attach"))

		goto out;



	do_test("bpf_fifo");



	bpf_link__destroy(link);

out:

	bpf_qdisc_fifo__destroy(fifo_skel);

}



static void test_fq(void)

{

	struct bpf_qdisc_fq *fq_skel;

	struct bpf_link *link;



	fq_skel = bpf_qdisc_fq__open_and_load();

	if (!ASSERT_OK_PTR(fq_skel, "bpf_qdisc_fq__open_and_load"))

		return;



	link = bpf_map__attach_struct_ops(fq_skel->maps.fq);

	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {

		bpf_qdisc_fq__destroy(fq_skel);

		return;

	}

	if (!ASSERT_OK(bpf_qdisc_fq__attach(fq_skel), "bpf_qdisc_fq__attach"))

		goto out;



	do_test("bpf_fq");



	bpf_link__destroy(link);

out:

	bpf_qdisc_fq__destroy(fq_skel);

}
@@ -96,18 +87,14 @@ static void test_qdisc_attach_to_mq(void) 
			    .handle = 0x11 << 16,

			    .qdisc = "bpf_fifo");

	struct bpf_qdisc_fifo *fifo_skel;

	struct bpf_link *link;

	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;

	}

	if (!ASSERT_OK(bpf_qdisc_fifo__attach(fifo_skel), "bpf_qdisc_fifo__attach"))

		goto out;



	SYS(out, "ip link add veth0 type veth peer veth1");

	hook.ifindex = if_nametoindex("veth0");
@@ -120,7 +107,6 @@ static void test_qdisc_attach_to_mq(void) 


	SYS(out, "tc qdisc delete dev veth0 root mq");

out:

	bpf_link__destroy(link);

	bpf_qdisc_fifo__destroy(fifo_skel);

}
@@ -132,18 +118,14 @@ static void test_qdisc_attach_to_non_root(void) 
			    .handle = 0x11 << 16,

			    .qdisc = "bpf_fifo");

	struct bpf_qdisc_fifo *fifo_skel;

	struct bpf_link *link;

	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;

	}

	if (!ASSERT_OK(bpf_qdisc_fifo__attach(fifo_skel), "bpf_qdisc_fifo__attach"))

		goto out;



	SYS(out, "tc qdisc add dev lo root handle 1: htb");

	SYS(out_del_htb, "tc class add dev lo parent 1: classid 1:1 htb rate 75Kbit");
@@ -155,18 +137,82 @@ static void test_qdisc_attach_to_non_root(void) 
out_del_htb:

	SYS(out, "tc qdisc delete dev lo root htb");

out:

	bpf_link__destroy(link);

	bpf_qdisc_fifo__destroy(fifo_skel);

}



void test_bpf_qdisc(void)

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;

	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 netns_obj *netns;

	char default_qdisc[IFNAMSIZ] = {};

	struct bpf_qdisc_fifo *fifo_skel;

	struct netns_obj *netns = NULL;

	int err;



	fifo_skel = bpf_qdisc_fifo__open_and_load();

	if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))

		return;



	if (!ASSERT_OK(bpf_qdisc_fifo__attach(fifo_skel), "bpf_qdisc_fifo__attach"))

		goto out;



	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"))

		return;

		goto out;



	SYS(out, "ip link add veth0 type veth peer veth1");

	SYS(out, "tc qdisc add dev veth0 root handle 1: mq");



	ASSERT_EQ(fifo_skel->bss->init_called, true, "init_called");



	SYS(out, "tc qdisc delete dev veth0 root mq");

out:

	netns_free(netns);

	if (default_qdisc[0])

		write_sysctl("/proc/sys/net/core/default_qdisc", default_qdisc);



	bpf_qdisc_fifo__destroy(fifo_skel);

}



void test_ns_bpf_qdisc(void)

{

	if (test__start_subtest("fifo"))

		test_fifo();

	if (test__start_subtest("fq"))
@@ -175,6 +221,11 @@ 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);

void serial_test_bpf_qdisc_default(void)

{

	test_default_qdisc_attach_to_mq();

}

tools/testing/selftests/bpf/progs/bpf_qdisc_common.h

+0 −6
Original line number Diff line number Diff line
@@ -14,12 +14,6 @@ 


struct bpf_sk_buff_ptr;



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;

tools/testing/selftests/bpf/progs/bpf_qdisc_fail__incompl_ops.c

0 → 100644
+41 −0
Original line number Diff line number Diff line 
// 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")

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")

struct sk_buff *BPF_PROG(bpf_qdisc_test_dequeue, struct Qdisc *sch)

{

	return NULL;

}



SEC("struct_ops")

void BPF_PROG(bpf_qdisc_test_reset, struct Qdisc *sch)

{

}



SEC("struct_ops")

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",

};

tools/testing/selftests/bpf/progs/bpf_qdisc_fifo.c

+9 −0
Original line number Diff line number Diff line
@@ -14,6 +14,8 @@ struct skb_node { 
private(A) struct bpf_spin_lock q_fifo_lock;

private(A) struct bpf_list_head q_fifo __contains(skb_node, node);



bool init_called;



SEC("struct_ops/bpf_fifo_enqueue")

int BPF_PROG(bpf_fifo_enqueue, struct sk_buff *skb, struct Qdisc *sch,

	     struct bpf_sk_buff_ptr *to_free)
@@ -77,6 +79,7 @@ int BPF_PROG(bpf_fifo_init, struct Qdisc *sch, struct nlattr *opt, 
	     struct netlink_ext_ack *extack)

{

	sch->limit = 1000;

	init_called = true;

	return 0;

}
@@ -106,12 +109,18 @@ void BPF_PROG(bpf_fifo_reset, struct Qdisc *sch) 
	sch->q.qlen = 0;

}



SEC("struct_ops")

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",

};