netdev - Re: [PATCH bpf-next v1 2/3] selftests/bpf: Add multi_st

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: <573da832-260f-4fc5-8e8c-38f185e09249@linux.dev>
Date: Mon, 4 Aug 2025 16:43:05 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>
Cc: netdev@...r.kernel.org, alexei.starovoitov@...il.com, andrii@...nel.org,
 daniel@...earbox.net, tj@...nel.org, memxor@...il.com,
 martin.lau@...nel.org, kernel-team@...a.com, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v1 2/3] selftests/bpf: Add multi_st_ops that
 supports multiple instances

On 7/31/25 2:09 PM, Amery Hung wrote:
> +static int multi_st_ops_reg(void *kdata, struct bpf_link *link)
> +{
> +	struct bpf_testmod_multi_st_ops *st_ops =
> +		(struct bpf_testmod_multi_st_ops *)kdata;
> +	struct bpf_map *map;
> +	unsigned long flags;
> +	int err = 0;
> +
> +	map = bpf_struct_ops_get(kdata);

The bpf_struct_ops_get returns a map pointer and also inc_not_zero() the map 
which we know it won't fail at this point, so no check is needed.

> +
> +	spin_lock_irqsave(&multi_st_ops_lock, flags);
> +	if (multi_st_ops_find_nolock(map->id)) {
> +		pr_err("multi_st_ops(id:%d) has already been registered\n", map->id);
> +		err = -EEXIST;
> +		goto unlock;
> +	}
> +
> +	st_ops->id = map->id;
> +	hlist_add_head(&st_ops->node, &multi_st_ops_list);
> +unlock:
> +	bpf_struct_ops_put(kdata);

To get an id, it needs a get and an immediate put. No concern on the performance 
  but just feels not easy to use. e.g. For the subsystem supporting link_update, 
it will need to do this get/put twice. One on the old kdata and another on the 
new kdata. Take a look at the bpf_struct_ops_map_link_update().

To create a id->struct_ops mapping, the subsystem needs neither the map pointer 
nor incrementing the map refcnt. How about create a new helper to only return 
the id of the kdata?

Uncompiled code:

u32 bpf_struct_ops_id(const void *kdata)
{
	struct bpf_struct_ops_value *kvalue;
	struct bpf_struct_ops_map *st_map;

	kvalue = container_of(kdata, struct bpf_struct_ops_value, data);
	st_map = container_of(kvalue, struct bpf_struct_ops_map, kvalue);

	return st_map->map.id;
}

> +	spin_unlock_irqrestore(&multi_st_ops_lock, flags);
> +
> +	return err;
> +}
> +
> +static void multi_st_ops_unreg(void *kdata, struct bpf_link *link)
> +{
> +	struct bpf_testmod_multi_st_ops *st_ops;
> +	struct bpf_map *map;
> +	unsigned long flags;
> +
> +	map = bpf_struct_ops_get(kdata);
> +
> +	spin_lock_irqsave(&multi_st_ops_lock, flags);
> +	st_ops = multi_st_ops_find_nolock(map->id);
> +	if (st_ops)
> +		hlist_del(&st_ops->node);
> +	spin_unlock_irqrestore(&multi_st_ops_lock, flags);
> +
> +	bpf_struct_ops_put(kdata);
> +}