Re: [Patch net-next] sock_map: dump socket map id via diag

[Ido Schimmel <idosch@idosch.org>]

On Sat, Feb 11, 2023 at 12:19:54PM -0800, Cong Wang wrote:
> +int sock_map_idiag_dump(struct sock *sk, struct sk_buff *skb, int attrtype)
> +{
> +	struct sk_psock_link *link;
> +	struct nlattr *nla, *attr;
> +	int nr_links = 0, ret = 0;
> +	struct sk_psock *psock;
> +	u32 *ids;
> +
> +	rcu_read_lock();
> +	psock = sk_psock_get(sk);
> +	if (unlikely(!psock)) {
> +		rcu_read_unlock();
> +		return 0;
> +	}
> +
> +	nla = nla_nest_start_noflag(skb, attrtype);

Since 'INET_DIAG_SOCKMAP' is a new attribute, did you consider using
nla_nest_start() instead?

> +	if (!nla) {
> +		sk_psock_put(sk, psock);
> +		rcu_read_unlock();
> +		return -EMSGSIZE;
> +	}
> +	spin_lock_bh(&psock->link_lock);
> +	list_for_each_entry(link, &psock->link, list)
> +		nr_links++;
> +
> +	attr = nla_reserve(skb, SK_DIAG_BPF_SOCKMAP_MAP_ID,
> +			   sizeof(link->map->id) * nr_links);
> +	if (!attr) {
> +		ret = -EMSGSIZE;
> +		goto unlock;
> +	}
> +
> +	ids = nla_data(attr);
> +	list_for_each_entry(link, &psock->link, list) {
> +		*ids = link->map->id;
> +		ids++;
> +	}

No strong preferences, but I think a more "modern" netlink usage would
be to encode each ID in a separate u32 attribute rather than encoding an
array of u32 in a single attribute. Example:

[ INET_DIAG_SOCKMAP ]	// nested
	[ SK_DIAG_BPF_SOCKMAP_MAP_ID ] // u32
	[ SK_DIAG_BPF_SOCKMAP_MAP_ID ] // u32
	...

Or:

[ INET_DIAG_SOCKMAP ]	// nested
	[ SK_DIAG_BPF_SOCKMAP_MAP_IDS ] // nested
		[ SK_DIAG_BPF_SOCKMAP_MAP_ID ] // u32
		[ SK_DIAG_BPF_SOCKMAP_MAP_ID ] // u32
		...

> +unlock:
> +	spin_unlock_bh(&psock->link_lock);
> +	sk_psock_put(sk, psock);
> +	rcu_read_unlock();
> +	if (ret)
> +		nla_nest_cancel(skb, nla);
> +	else
> +		nla_nest_end(skb, nla);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(sock_map_idiag_dump);