netdev - Re: [PATCH net-next v5] rtnetlink: Support fine-grained netdevice bulk deletion

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Dec 2021 23:47:12 +0200
From:   Lahav Schlesinger <lschlesinger@...venets.com>
To:     David Ahern <dsahern@...il.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, nikolay@...dia.com
Subject: Re: [PATCH net-next v5] rtnetlink: Support fine-grained netdevice
 bulk deletion

On Mon, Dec 06, 2021 at 09:12:33PM -0700, David Ahern wrote:
> CAUTION: External E-Mail - Use caution with links and attachments
>
>
> On 12/5/21 2:36 AM, Lahav Schlesinger wrote:
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index fd030e02f16d..5165cc699d97 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -37,6 +37,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/etherdevice.h>
> >  #include <linux/bpf.h>
> > +#include <linux/sort.h>
> >
> >  #include <linux/uaccess.h>
> >
> > @@ -1880,6 +1881,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
> >       [IFLA_PROTO_DOWN_REASON] = { .type = NLA_NESTED },
> >       [IFLA_NEW_IFINDEX]      = NLA_POLICY_MIN(NLA_S32, 1),
> >       [IFLA_PARENT_DEV_NAME]  = { .type = NLA_NUL_STRING },
> > +     [IFLA_IFINDEX]          = { .type = NLA_S32 },
>
> you need to make sure this new attribute can not be used in setlink
> requests or getlink.

Right, I'll add it.

>
> >  };
> >
> >  static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
> > @@ -3050,6 +3052,78 @@ static int rtnl_group_dellink(const struct net *net, int group)
> >       return 0;
> >  }
> >
> > +static int dev_ifindex_cmp(const void *a, const void *b)
> > +{
> > +     struct net_device * const *dev1 = a, * const *dev2 = b;
>
> const struct net_device *dev1 = 1, *dev2 = b;

The array stores 'struct net_device *', and 'a' and 'b' are pointers to
elements in this array (so 'struct net_device **', ignoring constness)

>
> > +
> > +     return (*dev1)->ifindex - (*dev2)->ifindex;
> > +}
> > +
> > +static int rtnl_ifindex_dellink(struct net *net, struct nlattr *head, int len,
> > +                             struct netlink_ext_ack *extack)
> > +{
> > +     int i = 0, num_devices = 0, rem;
> > +     struct net_device **dev_list;
> > +     const struct nlattr *nla;
> > +     LIST_HEAD(list_kill);
> > +     int ret;
> > +
> > +     nla_for_each_attr(nla, head, len, rem) {
> > +             if (nla_type(nla) == IFLA_IFINDEX)
> > +                     num_devices++;
> > +     }
>
>
> The need to walk the list twice (3 really with the sort) to means the
> array solution is better.
>
> > +
> > +     dev_list = kmalloc_array(num_devices, sizeof(*dev_list), GFP_KERNEL);
> > +     if (!dev_list)
> > +             return -ENOMEM;
> > +
> > +     nla_for_each_attr(nla, head, len, rem) {
> > +             const struct rtnl_link_ops *ops;
> > +             struct net_device *dev;
> > +             int ifindex;
> > +
> > +             if (nla_type(nla) != IFLA_IFINDEX)
> > +                     continue;
>
> and this business too. We have arrays in other places
> (net/ipv4/nexthop.c), so this is not the first.
>
>
> > +
> > +             ifindex = nla_get_s32(nla);
> > +             ret = -ENODEV;
> > +             dev = __dev_get_by_index(net, ifindex);
> > +             if (!dev) {
> > +                     NL_SET_ERR_MSG_ATTR(extack, nla, "Unknown ifindex");
> > +                     goto out_free;
> > +             }
> > +
> > +             ret = -EOPNOTSUPP;
> > +             ops = dev->rtnl_link_ops;
> > +             if (!ops || !ops->dellink) {
> > +                     NL_SET_ERR_MSG_ATTR(extack, nla, "Device cannot be deleted");
> > +                     goto out_free;
> > +             }
> > +
> > +             dev_list[i++] = dev;
> > +     }
> > +
> > +     /* Sort devices, so we could skip duplicates */
> > +     sort(dev_list, num_devices, sizeof(*dev_list), dev_ifindex_cmp, NULL);
>
> how did this sort change the results? 10k compares and re-order has to
> add some overhead.

No visible changes from what I saw, this API is as fast as group
deletion. Maybe a few tens of milliseconds slower, but it's lost in the
noise.
I'll run more thorough benchmarks to get to a more conclusive conclusion.

Also just pointing out that the sort will be needed even if we pass an
array (IFLA_IFINDEX_LIST) instead.
Feels like CS 101, but do you have a better approach for detecting
duplicates in an array? I imagine a hash table will be slower as it will
need to allocate a node object for each device (assuming we don't want
to add a new hlist_node to 'struct net_device' just for this)

>
> > +
> > +     for (i = 0; i < num_devices; i++) {
> > +             struct net_device *dev = dev_list[i];
> > +
> > +             if (i != 0 && dev_list[i - 1]->ifindex == dev->ifindex)
>
>                 if (i && ...)
>
>
> I liked the array variant better. Jakub?