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

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: <d1aae414-6225-4a1f-86dd-c185ebfa978f@davidwei.uk>
Date: Fri, 26 Jan 2024 10:54:35 -0800
From: David Wei <dw@...idwei.uk>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Jiri Pirko <jiri@...nulli.us>, Sabrina Dubroca <sd@...asysnail.net>,
 netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to
 be connected

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