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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <d0cbaf41-7632-4430-a228-d56c70dd6aed@linux.dev>
Date: Wed, 18 Dec 2024 15:37:14 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <amery.hung@...edance.com>
Cc: bpf@...r.kernel.org, netdev@...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, xiyou.wangcong@...il.com, 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 12/13/24 3:29 PM, Amery Hung wrote:
> +static int bpf_qdisc_init_member(const struct btf_type *t,
> +				 const struct btf_member *member,
> +				 void *kdata, const void *udata)
> +{
> +	const struct Qdisc_ops *uqdisc_ops;
> +	struct Qdisc_ops *qdisc_ops;
> +	u32 moff;
> +
> +	uqdisc_ops = (const struct Qdisc_ops *)udata;
> +	qdisc_ops = (struct Qdisc_ops *)kdata;
> +
> +	moff = __btf_member_bit_offset(t, member) / 8;
> +	switch (moff) {
> +	case offsetof(struct Qdisc_ops, priv_size):
> +		if (uqdisc_ops->priv_size)

bpf_struct_ops_map_update_elem() has enforced non function pointer member must 
be zero if ->init_member() returns 0, so this check is unnecessary.

> +			return -EINVAL;
> +		return 1;
> +	case offsetof(struct Qdisc_ops, static_flags):
> +		if (uqdisc_ops->static_flags)

Same here.

case priv_size and static_flags should be not needed, just return 0.

> +			return -EINVAL;
> +		return 1;
> +	case offsetof(struct Qdisc_ops, peek):
> +		if (!uqdisc_ops->peek)

bpf_struct_ops_map_update_elem() will assign the trampoline (that will call the 
bpf prog) to qdisc_ops->peek if the "u"qdisc_ops->peek has the prog fd.

This test is not necessary also.

> +			qdisc_ops->peek = qdisc_peek_dequeued;

Always do this assignment



> +		return 1;

and return 0 here. Allow the bpf_struct_ops_map_update_elem() to do the needed 
fd testing instead and reassign the qdisc_ops->peek with the trampoline if needed.

> +	case offsetof(struct Qdisc_ops, id):
> +		if (bpf_obj_name_cpy(qdisc_ops->id, uqdisc_ops->id,
> +				     sizeof(qdisc_ops->id)) <= 0)
> +			return -EINVAL;
> +		return 1;
> +	}
> +
> +	return 0;
> +}