netdev - Re: [PATCH net-next v2 1/3] 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: <12f55a91-1f93-48a0-96b9-63f0c1ea36cb@davidwei.uk>
Date: Tue, 12 Dec 2023 16:37:16 -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 v2 1/3] netdevsim: allow two netdevsim ports to
 be connected

On 2023-12-12 12:24, Jakub Kicinski wrote:
> On Sat,  9 Dec 2023 17:04:46 -0800 David Wei wrote:
>> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
>> index bcbc1e19edde..3e4378e9dbee 100644
>> --- a/drivers/net/netdevsim/bus.c
>> +++ b/drivers/net/netdevsim/bus.c
>> @@ -364,3 +364,13 @@ void nsim_bus_exit(void)
>>  	driver_unregister(&nsim_driver);
>>  	bus_unregister(&nsim_bus);
>>  }
>> +
>> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id)
> 
> nit: s/get/find/ get sometimes implied taking a reference 

Will do.

> 
>> +{
>> +	struct nsim_bus_dev *nsim_bus_dev;
> 
> new line here, please checkpatch --strict

Sorry, will remember to do this next time.

> 
>> +	list_for_each_entry(nsim_bus_dev, &nsim_bus_dev_list, list) {
>> +		if (nsim_bus_dev->dev.id == id)
>> +			return nsim_bus_dev;
> 
> You must assume some lock is being held so that you can walk the list
> and return a meaningful value? :) Please figure out what caller has to
> hold and add an appropriate lockdep assert here.

Will address.

> 
>> +	}
>> +	return NULL;
>> +}
> 
>> +static ssize_t nsim_dev_peer_read(struct file *file, char __user *data,
>> +				  size_t count, loff_t *ppos)
>> +{
>> +	struct nsim_dev_port *nsim_dev_port;
>> +	struct netdevsim *peer;
>> +	unsigned int id, port;
>> +	char buf[23];
>> +	ssize_t len;
>> +
>> +	nsim_dev_port = file->private_data;
>> +	rcu_read_lock();
>> +	peer = rcu_dereference(nsim_dev_port->ns->peer);
>> +	if (!peer) {
>> +		len = scnprintf(buf, sizeof(buf), "\n");
> 
> Why not return 0?

No reason, wasn't sure what the norm is.

> 
>> +		goto out;
>> +	}
>> +
>> +	id = peer->nsim_bus_dev->dev.id;
>> +	port = peer->nsim_dev_port->port_index;
>> +	len = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
>> +
>> +out:
>> +	rcu_read_unlock();
>> +	return simple_read_from_buffer(data, count, ppos, buf, len);
>> +}
> 
>> @@ -417,3 +418,5 @@ struct nsim_bus_dev {
>>  
>>  int nsim_bus_init(void);
>>  void nsim_bus_exit(void);
>> +
>> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id);
> 
> nit: let this go before the module init/exit funcs, 3 lines up

Will address.