On Sun, Feb 12, 2023 at 01:35:18PM +0200, Ido Schimmel wrote:
> 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?
Yes, but other INET_DIAG_* attributes are not new, hence why ss.c still
uses parse_rtattr(), I am not sure whether it is okay to change it to
parse_rtattr_nested() just because of this.
>
> > + 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
> ...
How do we know how many ID's we have here? Note, INET_DIAG_SOCKMAP in
the future could have other attributes, so can't simply mark the end of
ID's.
>
> 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
> ...
Yet one more nested level...
I prefer the current layout.
Thanks.
