netdev - Re: [PATCH net-next v2 2/3] rtnetlink: add IFLA_PARENT_[DEV|DEV_BUS]

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: <CAMZdPi_tv_MDFbSAx-hKTBsoi9=u7hQxcWNBcer5LQ+J37zgdw@mail.gmail.com>
Date:   Fri, 11 Jun 2021 17:07:16 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Parav Pandit <parav@...dia.com>
Cc:     "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "johannes.berg@...el.com" <johannes.berg@...el.com>,
        "leon@...nel.org" <leon@...nel.org>,
        "m.chetan.kumar@...el.com" <m.chetan.kumar@...el.com>,
        Sergey Ryazanov <ryazanov.s.a@...il.com>
Subject: Re: [PATCH net-next v2 2/3] rtnetlink: add IFLA_PARENT_[DEV|DEV_BUS]_NAME

Hi Parav,

On Fri, 11 Jun 2021 at 15:01, Parav Pandit <parav@...dia.com> wrote:
>
>
>
> > From: Loic Poulain <loic.poulain@...aro.org>
> > Sent: Thursday, June 10, 2021 11:15 PM
> >
> > From: Johannes Berg <johannes.berg@...el.com>
> >
> > In some cases, for example in the upcoming WWAN framework changes,
> > there's no natural "parent netdev", so sometimes dummy netdevs are
> > created or similar. IFLA_PARENT_DEV_NAME is a new attribute intended to
> > contain a device (sysfs, struct device) name that can be used instead when
> > creating a new netdev, if the rtnetlink family implements it.
> >
> > As suggested by Parav Pandit, we also introduce
> > IFLA_PARENT_DEV_BUS_NAME attribute in order to uniquely identify a
> > device on the system (with bus/name pair).
> >
> > ip-link(8) support for the generic parent device attributes will help us avoid
> > code duplication, so no other link type will require a custom code to handle
> > the parent name attribute. E.g. the WWAN interface creation command will
> > looks like this:
> >
> > $ ip link add wwan0-1 parent-dev wwan0 type wwan channel-id 1
> >
> > So, some future subsystem (or driver) FOO will have an interface creation
> > command that looks like this:
> >
> > $ ip link add foo1-3 parent-dev foo1 type foo bar-id 3 baz-type Y
> >
> > Below is an example of dumping link info of a random device with these new
> > attributes:
> >
> > $ ip --details link show wlp0s20f3
> >   4: wlp0s20f3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc
> > noqueue
> >      state UP mode DORMANT group default qlen 1000
> >      ...
> >      parent_devname 0000:00:14.3 parent_busname pci
>
> Showing bus first followed device is more preferred approach to see hierarchy.
> Please change their sequence.
>
> You should drop "name" suffix.
> "parent_bus" and "parent_dev" are just fine.

Do you think it can also be dropped for the IFLA symbol
(IFLA_PARENT_DEV && IFLA_PARENT_BUS_NAME).

>
> >
> > Co-developed-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> > Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> > Co-developed-by: Loic Poulain <loic.poulain@...aro.org>
> > Signed-off-by: Loic Poulain <loic.poulain@...aro.org>
> > Suggested-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> > Signed-off-by: Johannes Berg <johannes.berg@...el.com>
> > ---
> >  v2: - Squashed Johannes and Sergey changes
> >      - Added IFLA_PARENT_DEV_BUS_NAME attribute
> >      - reworded commit message + introduce Sergey's comment
> >
> >  include/uapi/linux/if_link.h |  7 +++++++
> >  net/core/rtnetlink.c         | 12 ++++++++++++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index
> > a5a7f0e..4882e81 100644
> > --- a/include/uapi/linux/if_link.h
> > +++ b/include/uapi/linux/if_link.h
> > @@ -341,6 +341,13 @@ enum {
> >       IFLA_ALT_IFNAME, /* Alternative ifname */
> >       IFLA_PERM_ADDRESS,
> >       IFLA_PROTO_DOWN_REASON,
> > +
> > +     /* device (sysfs) name as parent, used instead
> > +      * of IFLA_LINK where there's no parent netdev
> > +      */
> > +     IFLA_PARENT_DEV_NAME,
> > +     IFLA_PARENT_DEV_BUS_NAME,
> > +
> >       __IFLA_MAX
> >  };
> >
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 92c3e43..32599f3
> > 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1821,6 +1821,16 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb,
> >       if (rtnl_fill_prop_list(skb, dev))
> >               goto nla_put_failure;
> >
> > +     if (dev->dev.parent &&
> > +         nla_put_string(skb, IFLA_PARENT_DEV_NAME,
> > +                        dev_name(dev->dev.parent)))
> > +             goto nla_put_failure;
> > +
> > +     if (dev->dev.parent && dev->dev.parent->bus &&
> > +         nla_put_string(skb, IFLA_PARENT_DEV_BUS_NAME,
> > +                        dev->dev.parent->bus->name))
> > +             goto nla_put_failure;
> > +
> >       nlmsg_end(skb, nlh);
> >       return 0;
> >
> > @@ -1880,6 +1890,8 @@ static const struct nla_policy
> > ifla_policy[IFLA_MAX+1] = {
> >       [IFLA_PERM_ADDRESS]     = { .type = NLA_REJECT },
> >       [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_PARENT_DEV_BUS_NAME] = { .type = NLA_NUL_STRING },
> >  };
> >
> This hunk should go in the patch that enables users to use these fields to specify it for new link creation.

ack.

Thx,
Loic