netdev - RE: [PATCH net-next 3/4] rtnetlink: fill IFLA_PARENT_DEV

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] [day] [month] [year] [list]
Message-ID: <DM8PR12MB548073F28EEC62149EEB502FDC369@DM8PR12MB5480.namprd12.prod.outlook.com>
Date:   Wed, 9 Jun 2021 18:56:23 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>
CC:     Loic Poulain <loic.poulain@...aro.org>,
        "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>
Subject: RE: [PATCH net-next 3/4] rtnetlink: fill IFLA_PARENT_DEV_NAME on link
 dump

Hi Sergey,

> From: Sergey Ryazanov <ryazanov.s.a@...il.com>
> Sent: Wednesday, June 9, 2021 5:00 AM
> 
> Hello Parav,
> 
> On Tue, Jun 8, 2021 at 7:54 PM Parav Pandit <parav@...dia.com> wrote:
> >> From: Loic Poulain <loic.poulain@...aro.org>
> >> Sent: Tuesday, June 8, 2021 7:37 PM
> >>
> >> From: Sergey Ryazanov <ryazanov.s.a@...il.com>
> >>
> >> Return a parent device using the FLA_PARENT_DEV_NAME attribute
> during
> >> links dump. This should help a user figure out which links belong to
> >> a particular HW device. E.g. what data channels exists on a specific
> >> WWAN modem.
> >>
> > Please add the output sample in the commit message, for this additional
> field possibly for a more common netdevice of a pci device or some other
> one.
> >
> >> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> >> ---
> >>  net/core/rtnetlink.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index
> >> 56ac16a..120887c
> >> 100644
> >> --- a/net/core/rtnetlink.c
> >> +++ b/net/core/rtnetlink.c
> >> @@ -1819,6 +1819,11 @@ 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;
> >> +
> > A device name along with device bus establishes a unique identity in the
> system.
> 
> Sure. To uniquely identify an abstract device we need a full path, including a
> device parent. In sysfs it will be a device bus. But IFLA_PARENT_DEV_NAME
> was introduced to identify the parent device within a scope of a specific
> subsystem, which is specified by the IFLA_INFO_KIND attribute.

IFLA_INFO_KIND is not set for many types of netdevices which are not created by ip link add command.
Such as pci devices, auxiliary bus pci sf devices and possibly others.
IFLA_PARENT_DEV_NAME is returned for all netdevices whichever has it valid.

For example, for a netdevice with PCI parent, it will return as 0000:03:00.0.
This number string is useless without telling that it is pci device name.
So if you prefer to add PARENT_DEV_NAME, it needs to accompany along with its BUS_NAME too.
IFLA_INF_KIND for pci and other device is null.
So only PARENT_DEV is not sufficient.

> IFLA_PARENT_DEV_NAME should become a sane alternative for the
> IFLA_LINK usage when the link parent is not a netdev themself.
>
Ok. but not enough. It needs to accompany with the bus name too.
 
> You can find more details in the description of IFLA_PARENT_DEV_NAME
> introduction patch [1], my explanation, why we need to make the attribute
> common [2] and see a usage example in the wwan interface creation patch
> [3].
> 
I am not against making it common. I just say that if you prefer to expose this duplicate (and useful) info, please accompany with device bus name too.

Btw: parent device info is available via ethool such as

ethtool -i enp1s0f0 | grep bus-info
bus-info: 0000:01:00.0

This is left to the individual driver to fill up.
Compare to that generic way like this in this patch is desired with bus name.

> 1. https://lore.kernel.org/netdev/1623161227-29930-2-git-send-email-
> loic.poulain@...aro.org/
> 2.
> https://lore.kernel.org/netdev/CAHNKnsTKfFF9EckwSnLrwaPdH_tkjvdB3PVra
> MD-OLqFdLmp_Q@...l.gmail.com/
> 3. https://lore.kernel.org/netdev/1623161227-29930-4-git-send-email-
> loic.poulain@...aro.org/
> 
> > Hence you should add IFLA_PARENT_DEV_BUS_NAME and return it
> optionally if the device is on a bus.
> > If (dev->dev.parent->bus)
> >  return parent->bus->name string.
> 
> Looks like we are able to export the device bus name. Do you have a use case
> for this attribute? 
The one I explained above.

> And even so, should we bloat this simple patch with
> auxiliary attributes?
>
Not sure which one. I don’t see any more to add other that bus name and parent device name.

A similar patch [4] to include parent device for rdma networking devices didn't make through because its readily available in sysfs.
 
[4] https://lore.kernel.org/linux-rdma/BY5PR12MB432246F3155BC1D440857629DCEB0@BY5PR12MB4322.namprd12.prod.outlook.com/