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

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: <Y+jO9vxrjdppeg9a@shredder>
Date:   Sun, 12 Feb 2023 13:35:18 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        Cong Wang <cong.wang@...edance.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Sitnicki <jakub@...udflare.com>
Subject: Re: [Patch net-next] sock_map: dump socket map id via diag

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);