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

On Mon, Dec 06, 2021 at 09:25:17AM +0100, Nicolas Dichtel wrote:

CAUTION: External E-Mail - Use caution with links and attachments


Le 05/12/2021 à 10:36, Lahav Schlesinger a écrit :
Some comments below, but please, keep the people who replied to previous
versions of this patch in cc.

[snip]

quoted

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index eebd3894fe89..68fcde9c0c5e 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h

@@ -348,6 +348,7 @@ enum {
      IFLA_PARENT_DEV_NAME,
      IFLA_PARENT_DEV_BUS_NAME,

+     IFLA_IFINDEX,

nit: maybe the previous blank line sit better after this new attribute (and
before __IFLA_MAX)?

Due to the comment above the previous 2 attributes, I think that by
removing this empty line it can be accidentally thought as if the new
attribute is part of this "block".
As for adding a new line before __IFLA_MAX, I wanted to preserve the
appearance we had before the IFLA_PARENT_DEV_xxx attributes where added,
where there was no empty line before __IFLA_MAX.

I don't mind either way though, whatever looks better to you.

quoted

      __IFLA_MAX
 };

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 },

Same policy than IFLA_NEW_IFINDEX to refuse negative ifindex.

Right, thanks

quoted

 };

 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;
+
+     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++;
+     }
+
+     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;
+
+             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");

It would be nice to have the ifindex in the error message. This message does not
give more information than "ENODEV".

quoted

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

Same here.


Thank you,
Nicolas