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