[PATCH bpf-next/net] bpf: net_sched: Fix using bpf qdisc as default qdisc

[PATCH bpf-next/net] bpf: net_sched: Fix using bpf qdisc as default qdisc

[Amery Hung]

Use bpf_try_module_get()/bpf_module_put() instead of try_module_get()/
module_put() when handling default qdisc since users can assign a bpf
qdisc to it.

To trigger the bug:
$ bpftool struct_ops register bpf_qdisc_fq.bpf.o /sys/fs/bpf
$ echo bpf_fq > /proc/sys/net/core/default_qdisc

Fixes: c8240344956e (bpf: net_sched: Support implementation of Qdisc_ops in bpf)
Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 net/sched/sch_api.c     | 4 ++--
 net/sched/sch_generic.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index db6330258dda..1cda7e7feb32 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -208,7 +208,7 @@ static struct Qdisc_ops *qdisc_lookup_default(const char *name)
 
 	for (q = qdisc_base; q; q = q->next) {
 		if (!strcmp(name, q->id)) {
-			if (!try_module_get(q->owner))
+			if (!bpf_try_module_get(q, q->owner))
 				q = NULL;
 			break;
 		}
@@ -238,7 +238,7 @@ int qdisc_set_default(const char *name)
 
 	if (ops) {
 		/* Set new default */
-		module_put(default_qdisc_ops->owner);
+		bpf_module_put(ops, default_qdisc_ops->owner);
 		default_qdisc_ops = ops;
 	}
 	write_unlock(&qdisc_mod_lock);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index e6fda9f20272..7d2836d66043 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1002,14 +1002,14 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
 {
 	struct Qdisc *sch;
 
-	if (!try_module_get(ops->owner)) {
+	if (!bpf_try_module_get(ops, ops->owner)) {
 		NL_SET_ERR_MSG(extack, "Failed to increase module reference counter");
 		return NULL;
 	}
 
 	sch = qdisc_alloc(dev_queue, ops, extack);
 	if (IS_ERR(sch)) {
-		module_put(ops->owner);
+		bpf_module_put(ops, ops->owner);
 		return NULL;
 	}
 	sch->parent = parentid;
-- 
2.47.1

Re: [PATCH bpf-next/net] bpf: net_sched: Fix using bpf qdisc as default qdisc

[Cong Wang]

On Tue, Apr 22, 2025 at 03:58:08PM -0700, Amery Hung wrote:
> Use bpf_try_module_get()/bpf_module_put() instead of try_module_get()/
> module_put() when handling default qdisc since users can assign a bpf
> qdisc to it.
> 
> To trigger the bug:
> $ bpftool struct_ops register bpf_qdisc_fq.bpf.o /sys/fs/bpf
> $ echo bpf_fq > /proc/sys/net/core/default_qdisc
> 

Good to see eBPF Qdisc's can be default as well.

> Fixes: c8240344956e (bpf: net_sched: Support implementation of Qdisc_ops in bpf)
> Signed-off-by: Amery Hung <ameryhung@gmail.com>

Acked-by: Cong Wang <xiyou.wangcong@gmail.com>

Thanks!

Re: [PATCH bpf-next/net] bpf: net_sched: Fix using bpf qdisc as default qdisc

[Martin KaFai Lau]

On 4/22/25 3:58 PM, Amery Hung wrote:
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index db6330258dda..1cda7e7feb32 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -208,7 +208,7 @@ static struct Qdisc_ops *qdisc_lookup_default(const char *name)
>   
>   	for (q = qdisc_base; q; q = q->next) {
>   		if (!strcmp(name, q->id)) {
> -			if (!try_module_get(q->owner))
> +			if (!bpf_try_module_get(q, q->owner))
>   				q = NULL;
>   			break;
>   		}
> @@ -238,7 +238,7 @@ int qdisc_set_default(const char *name)
>   
>   	if (ops) {
>   		/* Set new default */
> -		module_put(default_qdisc_ops->owner);
> +		bpf_module_put(ops, default_qdisc_ops->owner);

The first arg, should it be the "default_qdisc_ops" instead?


>   		default_qdisc_ops = ops;
>   	}
>   	write_unlock(&qdisc_mod_lock);

Re: [PATCH bpf-next/net] bpf: net_sched: Fix using bpf qdisc as default qdisc

[Amery Hung]

On Tue, Apr 29, 2025 at 10:30 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 4/22/25 3:58 PM, Amery Hung wrote:
> > diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> > index db6330258dda..1cda7e7feb32 100644
> > --- a/net/sched/sch_api.c
> > +++ b/net/sched/sch_api.c
> > @@ -208,7 +208,7 @@ static struct Qdisc_ops *qdisc_lookup_default(const char *name)
> >
> >       for (q = qdisc_base; q; q = q->next) {
> >               if (!strcmp(name, q->id)) {
> > -                     if (!try_module_get(q->owner))
> > +                     if (!bpf_try_module_get(q, q->owner))
> >                               q = NULL;
> >                       break;
> >               }
> > @@ -238,7 +238,7 @@ int qdisc_set_default(const char *name)
> >
> >       if (ops) {
> >               /* Set new default */
> > -             module_put(default_qdisc_ops->owner);
> > +             bpf_module_put(ops, default_qdisc_ops->owner);
>
> The first arg, should it be the "default_qdisc_ops" instead?
>

You are right. I will fix this sloppy mistake and resend.

Thanks,
Amery

>
> >               default_qdisc_ops = ops;
> >       }
> >       write_unlock(&qdisc_mod_lock);