netdev - Re: [RFC PATCH v9 06/11] bpf: net

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <47a1dae1-7196-4991-b008-b50fb92fd5c3@linux.dev>
Date: Thu, 25 Jul 2024 15:38:54 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, yangpeihao@...u.edu.cn,
 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, xiyou.wangcong@...il.com,
 yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v9 06/11] bpf: net_sched: Add bpf qdisc kfuncs

On 7/14/24 10:51 AM, Amery Hung wrote:
> Add kfuncs for working on skb in qdisc.
> 
> Both bpf_qdisc_skb_drop() and bpf_skb_release() can be used to release
> a reference to an skb. However, bpf_qdisc_skb_drop() can only be called
> in .enqueue where a to_free skb list is available from kernel to defer

Enforcing the bpf_qdisc_skb_drop() kfunc only available to the ".enqueue" is 
achieved by the  "struct bpf_sk_buff_ptr" pointer type only available to the 
".enqueue" ops ?

> the release. Otherwise, bpf_skb_release() should be used elsewhere. It
> is also used in bpf_obj_free_fields() when cleaning up skb in maps and
> collections.
> 
> bpf_qdisc_schedule() can be used to schedule the execution of the qdisc.
> An example use case is to throttle a qdisc if the time to dequeue the
> next packet is known.
> 
> bpf_skb_get_hash() returns the flow hash of an skb, which can be used
> to build flow-based queueing algorithms.
> 
> Signed-off-by: Amery Hung <amery.hung@...edance.com>
> ---
>   net/sched/bpf_qdisc.c | 74 ++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
> index a68fc115d8f8..eff7559aa346 100644
> --- a/net/sched/bpf_qdisc.c
> +++ b/net/sched/bpf_qdisc.c
> @@ -148,6 +148,64 @@ static int bpf_qdisc_btf_struct_access(struct bpf_verifier_log *log,
>   	return 0;
>   }
>   
> +__bpf_kfunc_start_defs();
> +
> +/* bpf_skb_get_hash - Get the flow hash of an skb.
> + * @skb: The skb to get the flow hash from.
> + */
> +__bpf_kfunc u32 bpf_skb_get_hash(struct sk_buff *skb)
> +{
> +	return skb_get_hash(skb);
> +}
> +
> +/* bpf_skb_release - Release an skb reference acquired on an skb immediately.
> + * @skb: The skb on which a reference is being released.
> + */
> +__bpf_kfunc void bpf_skb_release(struct sk_buff *skb)
> +{
> +	consume_skb(skb);

snippet from the comment of consume_skb():

  *      Functions identically to kfree_skb, but kfree_skb assumes that the frame
  *      is being dropped after a failure and notes that

consume_skb() has a different tracepoint from the kfree_skb also. It is better 
not to confuse the tracing.

I think at least the Qdisc_ops.reset and the btf_id_dtor_kfunc don't fall into 
the consume_skb(). May be useful to add the kfree_skb[_reason?]() kfunc also?

> +}
> +
> +/* bpf_qdisc_skb_drop - Add an skb to be dropped later to a list.
> + * @skb: The skb on which a reference is being released and dropped.
> + * @to_free_list: The list of skbs to be dropped.
> + */
> +__bpf_kfunc void bpf_qdisc_skb_drop(struct sk_buff *skb,
> +				    struct bpf_sk_buff_ptr *to_free_list)
> +{
> +	__qdisc_drop(skb, (struct sk_buff **)to_free_list);
> +}
> +
> +/* bpf_qdisc_watchdog_schedule - Schedule a qdisc to a later time using a timer.
> + * @sch: The qdisc to be scheduled.
> + * @expire: The expiry time of the timer.
> + * @delta_ns: The slack range of the timer.
> + */
> +__bpf_kfunc void bpf_qdisc_watchdog_schedule(struct Qdisc *sch, u64 expire, u64 delta_ns)
> +{
> +	struct bpf_sched_data *q = qdisc_priv(sch);
> +
> +	qdisc_watchdog_schedule_range_ns(&q->watchdog, expire, delta_ns);
> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +BTF_KFUNCS_START(bpf_qdisc_kfunc_ids)
> +BTF_ID_FLAGS(func, bpf_skb_get_hash)

Add KF_TRUSTED_ARGS. Avoid cases like getting a skb from walking the skb->next 
for now.

> +BTF_ID_FLAGS(func, bpf_skb_release, KF_RELEASE)
> +BTF_ID_FLAGS(func, bpf_qdisc_skb_drop, KF_RELEASE)
> +BTF_ID_FLAGS(func, bpf_qdisc_watchdog_schedule)

Also add KF_TRUSTED_ARGS here.

> +BTF_KFUNCS_END(bpf_qdisc_kfunc_ids)
> +
> +static const struct btf_kfunc_id_set bpf_qdisc_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set   = &bpf_qdisc_kfunc_ids,
> +};
> +
> +BTF_ID_LIST(skb_kfunc_dtor_ids)
> +BTF_ID(struct, sk_buff)
> +BTF_ID_FLAGS(func, bpf_skb_release, KF_RELEASE)
> +
>   static const struct bpf_verifier_ops bpf_qdisc_verifier_ops = {
>   	.get_func_proto		= bpf_qdisc_get_func_proto,
>   	.is_valid_access	= bpf_qdisc_is_valid_access,
> @@ -347,6 +405,20 @@ static struct bpf_struct_ops bpf_Qdisc_ops = {
>   
>   static int __init bpf_qdisc_kfunc_init(void)
>   {
> -	return register_bpf_struct_ops(&bpf_Qdisc_ops, Qdisc_ops);
> +	int ret;
> +	const struct btf_id_dtor_kfunc skb_kfunc_dtors[] = {
> +		{
> +			.btf_id       = skb_kfunc_dtor_ids[0],
> +			.kfunc_btf_id = skb_kfunc_dtor_ids[1]
> +		},
> +	};
> +
> +	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &bpf_qdisc_kfunc_set);
> +	ret = ret ?: register_btf_id_dtor_kfuncs(skb_kfunc_dtors,
> +						 ARRAY_SIZE(skb_kfunc_dtors),
> +						 THIS_MODULE);
> +	ret = ret ?: register_bpf_struct_ops(&bpf_Qdisc_ops, Qdisc_ops);
> +
> +	return ret;
>   }
>   late_initcall(bpf_qdisc_kfunc_init);