Re: [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 25 Jan 2024 17:23:54 -0800 David Wei wrote:
> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> index bcbc1e19edde..be8ac2e60c69 100644
> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -232,9 +232,81 @@ del_device_store(const struct bus_type *bus, const char *buf, size_t count)
>  }
>  static BUS_ATTR_WO(del_device);
>  
> +static ssize_t link_device_store(const struct bus_type *bus, const char *buf, size_t count)
> +{
> +	unsigned int netnsid_a, netnsid_b, ifidx_a, ifidx_b;
> +	struct netdevsim *nsim_a, *nsim_b;
> +	struct net_device *dev_a, *dev_b;
> +	struct net *ns_a, *ns_b;
> +	int err;
> +
> +	err = sscanf(buf, "%u %u %u %u", &netnsid_a, &ifidx_a, &netnsid_b, &ifidx_b);

I'd go for "%u:%u %u:%u" to make the 'grouping' of netns and ifindex
more obvious. But no strong feelings.

> +	if (err != 4) {
> +		pr_err("Format for linking two devices is \"netnsid_a ifidx_a netnsid_b ifidx_b\" (uint uint unit uint).\n");
> +		return -EINVAL;
> +	}
> +
> +	err = -EINVAL;
> +	rtnl_lock();
> +	ns_a = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_a);
> +	if (!ns_a) {
> +		pr_err("Could not find netns with id: %d\n", netnsid_a);
> +		goto out_unlock_rtnl;
> +	}
> +
> +	dev_a = dev_get_by_index(ns_a, ifidx_a);

since you're under rtnl_lock you can use __get_device_by_index(),
it doesn't increase the refcount so you won't have to worry about
releasing it.

> +	if (!dev_a) {
> +		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_a, netnsid_a);
> +		goto out_put_netns_a;
> +	}
> +
> +	if (!netdev_is_nsim(dev_a)) {
> +		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_a, netnsid_a);
> +		goto out_put_dev_a;
> +	}
> +
> +	ns_b = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_b);
> +	if (!ns_b) {
> +		pr_err("Could not find netns with id: %d\n", netnsid_b);
> +		goto out_put_dev_a;
> +	}
> +
> +	dev_b = dev_get_by_index(ns_b, ifidx_b);
> +	if (!dev_b) {
> +		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_b, netnsid_b);
> +		goto out_put_netns_b;
> +	}
> +
> +	if (!netdev_is_nsim(dev_b)) {
> +		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_b, netnsid_b);
> +		goto out_put_dev_b;
> +	}
> +
> +	err = 0;
> +	nsim_a = netdev_priv(dev_a);
> +	nsim_b = netdev_priv(dev_b);
> +	rcu_assign_pointer(nsim_a->peer, nsim_b);
> +	rcu_assign_pointer(nsim_b->peer, nsim_a);

Shouldn't we check if peer is NULL? Otherwise we can get into weird
situations where we link A<>B then B<>C and then the pointers look like
this A->B<>C. When B gets freed A's pointer won't get cleared.

> +out_put_dev_b:
> +	dev_put(dev_b);
> +out_put_netns_b:
> +	put_net(ns_b);
> +out_put_dev_a:
> +	dev_put(dev_a);
> +out_put_netns_a:
> +	put_net(ns_a);
> +out_unlock_rtnl:
> +	rtnl_unlock();
> +
> +	return !err ? count : err;
> +}
> +static BUS_ATTR_WO(link_device);