On 12/9/24 15:01, Xiao Liang wrote:
> 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.
>
> +------------+-------------------+---------+---------+
> | 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. This has some side effects, including extra ifindex allocation,
> ifname validation and link notifications. There's also an extra step to
> move the device to target netns. These could be avoided if we create it
> in target netns at the beginning.
>
> On the other hand, the meaning of src_net 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 refactors netns handling by packing newlink() parameters into
> a struct, and passing source, link and peer netns as is through this
> struct. Fallback logic is implemented in helper functions -
> rtnl_newlink_link_net() and rtnl_newlink_peer_net(). If is not set, peer
> netns falls back to link netns, and link netns falls back to source netns.
> rtnl_newlink_create() now creates devices in target netns directly,
> so dev_net is always target netns.
>
> For drivers that use dev_net as fallback of link_netns, current behavior
> is kept for compatibility.
>
> Signed-off-by: Xiao Liang <shaw.leon@xxxxxxxxx>
I must admit this patch is way too huge for me to allow any reasonable
review except that this has the potential of breaking a lot of things.
I think you should be splitted to make it more palatable; i.e.
- a patch just add the params struct with no semantic changes.
- a patch making the dev_change_net_namespace() conditional on net !=
tge_net[1]
- many per-device patches creating directly the device in the target
namespace.
- a patch reverting [1]
Other may have different opinions, I'd love to hear them.
> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 98c6205ed19f..2f7bf50e05d2 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -3161,14 +3161,17 @@ static int amt_validate(struct nlattr *tb[], struct nlattr *data[],
> return 0;
> }
>
> -static int amt_newlink(struct net *net, struct net_device *dev,
> - struct nlattr *tb[], struct nlattr *data[],
> - struct netlink_ext_ack *extack)
> +static int amt_newlink(struct rtnl_newlink_params *params)
> {
> + struct net_device *dev = params->dev;
> + struct nlattr **tb = params->tb;
> + struct nlattr **data = params->data;
> + struct netlink_ext_ack *extack = params->extack;
> + struct net *link_net = rtnl_newlink_link_net(params);
> struct amt_dev *amt = netdev_priv(dev);
> int err = -EINVAL;
Minor nit: here and and many other places, please respect the reverse
xmas tree order.
Thanks,
Paolo
