netdev - Re: [PATCH bpf-next v1 05/13] bpf: net_sched: Support implementation of Qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z10OwXiU/RTx3Qbe@pop-os.localdomain>
Date: Fri, 13 Dec 2024 20:51:13 -0800
From: Cong Wang <xiyou.wangcong@...il.com>
To: Amery Hung <amery.hung@...edance.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, daniel@...earbox.net,
	andrii@...nel.org, alexei.starovoitov@...il.com,
	martin.lau@...nel.org, sinquersw@...il.com, toke@...hat.com,
	jhs@...atatu.com, jiri@...nulli.us, stfomichev@...il.com,
	ekarani.silvestre@....ufcg.edu.br, yangpeihao@...u.edu.cn,
	yepeilin.cs@...il.com, ameryhung@...il.com
Subject: Re: [PATCH bpf-next v1 05/13] bpf: net_sched: Support implementation
 of Qdisc_ops in bpf

On Fri, Dec 13, 2024 at 11:29:50PM +0000, Amery Hung wrote:
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 4214e76c9168..eb16218fdf52 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -563,6 +563,7 @@ const char *btf_name_by_offset(const struct btf *btf, u32 offset);
>  const char *btf_str_by_offset(const struct btf *btf, u32 offset);
>  struct btf *btf_parse_vmlinux(void);
>  struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> +u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto, int off);
>  u32 *btf_kfunc_id_set_contains(const struct btf *btf, u32 kfunc_btf_id,
>  			       const struct bpf_prog *prog);
>  u32 *btf_kfunc_is_modify_return(const struct btf *btf, u32 kfunc_btf_id,
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index a05ccf9ee032..f733dbf24261 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6375,8 +6375,8 @@ static bool is_int_ptr(struct btf *btf, const struct btf_type *t)
>  	return btf_type_is_int(t);
>  }
>  
> -static u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto,
> -			   int off)
> +u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto,
> +		    int off)
>  {
>  	const struct btf_param *args;
>  	const struct btf_type *t;

Maybe separate this piece out as a separate patch?


> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index 8180d0c12fce..ccd0255da5a5 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -403,6 +403,18 @@ config NET_SCH_ETS
>  
>  	  If unsure, say N.
>  
> +config NET_SCH_BPF
> +	bool "BPF-based Qdisc"
> +	depends on BPF_SYSCALL && BPF_JIT && DEBUG_INFO_BTF

I think new features should be default to n, unless you have reasons not
to do so.

> +	help
> +	  This option allows BPF-based queueing disiplines. With BPF struct_ops,
> +	  users can implement supported operators in Qdisc_ops using BPF programs.
> +	  The queue holding skb can be built with BPF maps or graphs.
> +
> +	  Say Y here if you want to use BPF-based Qdisc.
> +
> +	  If unsure, say N.
> +
>  menuconfig NET_SCH_DEFAULT
>  	bool "Allow override default queue discipline"
>  	help

[...]

> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index 2eefa4783879..f074053c4232 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -25,6 +25,7 @@
>  #include <linux/hrtimer.h>
>  #include <linux/slab.h>
>  #include <linux/hashtable.h>
> +#include <linux/bpf.h>
>  
>  #include <net/net_namespace.h>
>  #include <net/sock.h>
> @@ -358,7 +359,7 @@ static struct Qdisc_ops *qdisc_lookup_ops(struct nlattr *kind)
>  		read_lock(&qdisc_mod_lock);
>  		for (q = qdisc_base; q; q = q->next) {
>  			if (nla_strcmp(kind, q->id) == 0) {
> -				if (!try_module_get(q->owner))
> +				if (!bpf_try_module_get(q, q->owner))
>  					q = NULL;
>  				break;
>  			}
> @@ -1287,7 +1288,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>  				/* We will try again qdisc_lookup_ops,
>  				 * so don't keep a reference.
>  				 */
> -				module_put(ops->owner);
> +				bpf_module_put(ops, ops->owner);
>  				err = -EAGAIN;
>  				goto err_out;
>  			}
> @@ -1398,7 +1399,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>  	netdev_put(dev, &sch->dev_tracker);
>  	qdisc_free(sch);
>  err_out2:
> -	module_put(ops->owner);
> +	bpf_module_put(ops, ops->owner);
>  err_out:
>  	*errp = err;
>  	return NULL;
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 38ec18f73de4..1e770ec251a0 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -24,6 +24,7 @@
>  #include <linux/if_vlan.h>
>  #include <linux/skb_array.h>
>  #include <linux/if_macvlan.h>
> +#include <linux/bpf.h>
>  #include <net/sch_generic.h>
>  #include <net/pkt_sched.h>
>  #include <net/dst.h>
> @@ -1083,7 +1084,7 @@ static void __qdisc_destroy(struct Qdisc *qdisc)
>  		ops->destroy(qdisc);
>  
>  	lockdep_unregister_key(&qdisc->root_lock_key);
> -	module_put(ops->owner);
> +	bpf_module_put(ops, ops->owner);
>  	netdev_put(dev, &qdisc->dev_tracker);
>  
>  	trace_qdisc_destroy(qdisc);

Maybe this piece should be separated out too? Ideally this patch should
only have bpf_qdisc.c and its Makefile and Kconfig changes.

Regards.