netdev - Re: [patch net-next 3/3] netdevsim: create devlink and netdev instances in namespace

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: <20190730060655.GB2312@nanopsycho.orion>
Date:   Tue, 30 Jul 2019 08:06:55 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        sthemmin@...rosoft.com, dsahern@...il.com, mlxsw@...lanox.com
Subject: Re: [patch net-next 3/3] netdevsim: create devlink and netdev
 instances in namespace

Mon, Jul 29, 2019 at 08:59:06PM CEST, jakub.kicinski@...ronome.com wrote:
>On Sat, 27 Jul 2019 11:44:59 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@...lanox.com>
>> 
>> When user does create new netdevsim instance using sysfs bus file,
>> create the devlink instance and related netdev instance in the namespace
>> of the caller.
>> 
>> Signed-off-by: Jiri Pirko <jiri@...lanox.com>
>
>> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
>> index 1a0ff3d7747b..6aeed0c600f8 100644
>> --- a/drivers/net/netdevsim/bus.c
>> +++ b/drivers/net/netdevsim/bus.c
>> @@ -283,6 +283,7 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count)
>>  	nsim_bus_dev->dev.bus = &nsim_bus;
>>  	nsim_bus_dev->dev.type = &nsim_bus_dev_type;
>>  	nsim_bus_dev->port_count = port_count;
>> +	nsim_bus_dev->initial_net = current->nsproxy->net_ns;
>>  
>>  	err = device_register(&nsim_bus_dev->dev);
>>  	if (err)
>
>The saved initial_net is only to carry the net info from the adding
>process to the probe callback, because probe can be asynchronous?

Exactly.


>I'm not entirely sure netdevsim can probe asynchronously in practice
>given we never return EPROBE_DEFER, but would you mind at least adding
>a comment stating that next to the definition of the field, otherwise 
>I worry someone may mistakenly use this field instead of the up-to-date
>devlink's netns.

Will do.


>
>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>> index 79c05af2a7c0..cdf53d0e0c49 100644
>> --- a/drivers/net/netdevsim/netdevsim.h
>> +++ b/drivers/net/netdevsim/netdevsim.h
>> @@ -19,6 +19,7 @@
>>  #include <linux/netdevice.h>
>>  #include <linux/u64_stats_sync.h>
>>  #include <net/devlink.h>
>> +#include <net/net_namespace.h>
>
>You can just do a forward declaration, no need to pull in the header.

Sure, but why?


>
>>  #include <net/xdp.h>
>>  
>>  #define DRV_NAME	"netdevsim"
>
>> @@ -213,6 +215,7 @@ struct nsim_bus_dev {
>>  	struct device dev;
>>  	struct list_head list;
>>  	unsigned int port_count;
>> +	struct net *initial_net;
>>  	unsigned int num_vfs;
>>  	struct nsim_vf_config *vfconfigs;
>>  };
>
>Otherwise makes perfect sense, with the above nits addressed feel free
>to add:
>
>Acked-by: Jakub Kicinski <jakub.kicinski@...ronome.com>