Message-ID: <20250213065348.8507-1-kuniyu@amazon.com>
Date: Thu, 13 Feb 2025 15:53:48 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <shaw.leon@...il.com>
CC: <alex.aring@...il.com>, <andrew+netdev@...n.ch>,
	<b.a.t.m.a.n@...ts.open-mesh.org>, <bpf@...r.kernel.org>,
	<bridge@...ts.linux.dev>, <davem@...emloft.net>, <donald.hunter@...il.com>,
	<dsahern@...nel.org>, <edumazet@...gle.com>, <herbert@...dor.apana.org.au>,
	<horms@...nel.org>, <kuba@...nel.org>, <kuniyu@...zon.com>,
	<linux-can@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-kselftest@...r.kernel.org>, <linux-ppp@...r.kernel.org>,
	<linux-rdma@...r.kernel.org>, <linux-wireless@...r.kernel.org>,
	<linux-wpan@...r.kernel.org>, <miquel.raynal@...tlin.com>,
	<netdev@...r.kernel.org>, <osmocom-net-gprs@...ts.osmocom.org>,
	<pabeni@...hat.com>, <shuah@...nel.org>, <stefan@...enfreihafen.org>,
	<steffen.klassert@...unet.com>, <wireguard@...ts.zx2c4.com>
Subject: Re: [PATCH net-next v9 02/11] rtnetlink: Pack newlink() params into struct

From: Xiao Liang <shaw.leon@...il.com>
Date: Mon, 10 Feb 2025 21:29:53 +0800
> There are 4 net namespaces involved when creating links:
> 
>  - source netns - where the netlink socket resides,
>  - target netns - where to put the device being created,
>  - link netns - netns associated with the device (backend),
>  - peer netns - netns of peer device.
> 
> Currently, two nets are passed to newlink() callback - "src_net"
> parameter and "dev_net" (implicitly in net_device). They are set as
> follows, depending on netlink attributes in the request.
> 
>  +------------+-------------------+---------+---------+
>  | peer netns | IFLA_LINK_NETNSID | src_net | dev_net |
>  +------------+-------------------+---------+---------+
>  |            | absent            | source  | target  |
>  | absent     +-------------------+---------+---------+
>  |            | present           | link    | link    |
>  +------------+-------------------+---------+---------+
>  |            | absent            | peer    | target  |
>  | present    +-------------------+---------+---------+
>  |            | present           | peer    | link    |
>  +------------+-------------------+---------+---------+
> 
> When IFLA_LINK_NETNSID is present, the device is created in link netns
> first and then moved to target netns. This has some side effects,
> including extra ifindex allocation, ifname validation and link events.
> These could be avoided if we create it in target netns from
> the beginning.
> 
> On the other hand, the meaning of src_net parameter is ambiguous. It
> varies depending on how parameters are passed. It is the effective
> link (or peer netns) by design, but some drivers ignore it and use
> dev_net instead.
> 
> This patch packs existing newlink() parameters, along with the source
> netns, link netns and peer netns, into a struct. The old "src_net"
> is renamed to "net" to avoid confusion with real source netns, and
> will be deprecated later. The use of src_net are converted to
> params->net trivially.
> 
> To make the semantics more clear, two helper functions -
> rtnl_newlink_link_net() and rtnl_newlink_peer_net() - are provided
> for netns fallback logic. Peer netns falls back to link netns, and
> link netns falls back to source netns.
> 
> In following patches, to prepare for creating link in target netns
> directly:
> 
>   - For device drivers that are aware of the old "src_net", the use of
>     it are replace with one of the two helper functions.
>   - And for those that takes dev_net() as link netns, we try
>     params->link_net and then dev_net(), in order to maintain
>     compatibility with the old behavior.
> 
> Signed-off-by: Xiao Liang <shaw.leon@...il.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>

I left small comments below.


[...]
> diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
> index 523025106a64..0f7281e3e448 100644
> --- a/include/linux/if_macvlan.h
> +++ b/include/linux/if_macvlan.h
> @@ -59,8 +59,10 @@ static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
>  
>  extern void macvlan_common_setup(struct net_device *dev);
>  
> -extern int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
> -				  struct nlattr *tb[], struct nlattr *data[],
> +struct rtnl_newlink_params;

You can just include <net/rtnetlink.h> and remove it from .c
files, then this forward declaration will be unnecessary.


> +
> +extern int macvlan_common_newlink(struct net_device *dev,
> +				  struct rtnl_newlink_params *params,
>  				  struct netlink_ext_ack *extack);
>  
>  extern void macvlan_dellink(struct net_device *dev, struct list_head *head);


[...]
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index bc0069a8b6ea..00c086ca0c11 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -69,6 +69,42 @@ static inline int rtnl_msg_family(const struct nlmsghdr *nlh)
>  		return AF_UNSPEC;
>  }
>  
> +/**
> + *	struct rtnl_newlink_params - parameters of rtnl_link_ops::newlink()

The '\t' after '*' should be single '\s'.

Same for lines below.


> + *
> + *	@net: Netns of interest
> + *	@src_net: Source netns of rtnetlink socket
> + *	@link_net: Link netns by IFLA_LINK_NETNSID, NULL if not specified
> + *	@peer_net: Peer netns
> + *	@tb: IFLA_* attributes
> + *	@data: IFLA_INFO_DATA attributes
> + */
> +struct rtnl_newlink_params {

[...]
> +/* Get effective link netns from newlink params. Generally, this is link_net
> + * and falls back to src_net. But for compatibility, a driver may * choose to
> + * use dev_net(dev) instead.
> + */
> +static inline struct net *rtnl_newlink_link_net(struct rtnl_newlink_params *p)
> +{
> +	return p->link_net ? : p->src_net;
> +}
> +
> +/* Get peer netns from newlink params. Fallback to link netns if peer netns is
> + * not specified explicitly.
> + */
> +static inline struct net *rtnl_newlink_peer_net(struct rtnl_newlink_params *p)
> +{
> +	return p->peer_net ? : rtnl_newlink_link_net(p);
> +}

These helpers should belong to patch 2 ?

Powered by blists - more mailing lists