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

[David Wei <dw@davidwei.uk>]

On 2024-01-25 18:24, Jakub Kicinski wrote:
> 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.

Also no strong feelings so I will go with your 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.

Ah, I will change this. Is this true in general i.e. if I hold some big
lock then I can use versions of functions that do not modify refcounts?

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

Yep, that's an oversight from me. Will address.

> 
>> +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);