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: <PH0PR12MB5481986BF646806E23909CD7DC349@PH0PR12MB5481.namprd12.prod.outlook.com>
Date:   Fri, 11 Jun 2021 13:01:47 +0000
From:   Parav Pandit <parav@...dia.com>
To:     Loic Poulain <loic.poulain@...aro.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "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



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

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