netdev - Re: [PATCH net-next 2/2] rtnetlink: use xarray iterator to implement rtnl_dump

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]

Date: Sat, 10 Feb 2024 12:23:30 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S . Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	eric.dumazet@...il.com
Subject: Re: [PATCH net-next 2/2] rtnetlink: use xarray iterator to implement rtnl_dump_ifinfo()

On Fri, Feb 9, 2024 at 11:24 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Fri,  9 Feb 2024 14:56:15 +0000 Eric Dumazet wrote:
> > +     unsigned long ifindex = cb->args[0];
>
> [snip]
>
> > +     for_each_netdev_dump(tgt_net, dev, ifindex) {
> > +             if (link_dump_filtered(dev, master_idx, kind_ops))
> > +                     continue;
> > +             err = rtnl_fill_ifinfo(skb, dev, net, RTM_NEWLINK,
> > +                                    NETLINK_CB(cb->skb).portid,
> > +                                    nlh->nlmsg_seq, 0, flags,
> > +                                    ext_filter_mask, 0, NULL, 0,
> > +                                    netnsid, GFP_KERNEL);
> > +
> > +             if (err < 0)
> > +                     break;
> > +             cb->args[0] = ifindex + 1;
>
> Perhaps we can cast the context buffer onto something typed and use
> it directly? I think it's a tiny bit less error prone:
>
>         struct {
>                 unsigned long ifindex;
>         } *ctx = (void *)cb->ctx;
>
> Then we can:
>
>         for_each_netdev_dump(tgt_net, dev, ctx->ifindex)
>                                            ^^^^^^^^^^^^
>
> and not need to worry about saving the ifindex back to cb before
> exiting.

Hi Jakub

I tried something like that (adding a common structure for future
conversions), but this was not working properly.

Unfortunately we only can save the ifindex back to cb->XXXX only after
 rtnl_fill_ifinfo() was a success.

For instance, after applying the following diff to my patch, we have a
bug, because iip link loops on the last device.

We need to set cb->args[0] to  last_dev->ifindex + 1 to end the dump.

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 68a224bbf1dd6118526329782362a4bfc192d6b1..7f562d6e40ebf5329e9c0b1c7add81c461683907
100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2192,9 +2192,11 @@ static int rtnl_dump_ifinfo(struct sk_buff
*skb, struct netlink_callback *cb)
        struct netlink_ext_ack *extack = cb->extack;
        const struct nlmsghdr *nlh = cb->nlh;
        struct net *net = sock_net(skb->sk);
-       unsigned long ifindex = cb->args[0];
        unsigned int flags = NLM_F_MULTI;
        struct nlattr *tb[IFLA_MAX+1];
+       struct {
+               unsigned long ifindex;
+       } *ctx = (void *)cb->ctx;
        struct net *tgt_net = net;
        u32 ext_filter_mask = 0;
        struct net_device *dev;
@@ -2246,7 +2248,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb,
struct netlink_callback *cb)

 walk_entries:
        err = skb->len;
-       for_each_netdev_dump(tgt_net, dev, ifindex) {
+       for_each_netdev_dump(tgt_net, dev, ctx->ifindex) {
                if (link_dump_filtered(dev, master_idx, kind_ops))
                        continue;
                err = rtnl_fill_ifinfo(skb, dev, net, RTM_NEWLINK,
@@ -2257,7 +2259,6 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb,
struct netlink_callback *cb)

                if (err < 0)
                        break;
-               cb->args[0] = ifindex + 1;
        }
        cb->seq = tgt_net->dev_base_seq;
        nl_dump_check_consistent(cb, nlmsg_hdr(skb));