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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20211207124858.3tpsojcamyxldjb4@kgollan-pc>
Date:   Tue, 7 Dec 2021 14:48:59 +0200
From:   Lahav Schlesinger <lschlesinger@...venets.com>
To:     Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:     netdev@...r.kernel.org, kuba@...nel.org, dsahern@...il.com,
        nikolay@...dia.com
Subject: 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]
>
> > 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.

>
> >       __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

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