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  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]
Date:   Mon, 27 Jan 2020 20:04:14 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net v2 2/6] netdevsim: disable devlink reload when
 resources are being used

On Mon, 27 Jan 2020 14:30:15 +0000, Taehee Yoo wrote:
> devlink reload destroys resources and allocates resources again.
> So, when devices and ports resources are being used, devlink reload
> function should not be executed. In order to avoid this race, a new
> lock is added and new_port() and del_port() call devlink_reload_disable()
> and devlink_reload_enable().
> 
> Thread0                      Thread1
> {new/del}_port()             {new/del}_port()
> devlink_reload_disable()
>                              devlink_reload_disable()
> devlink_reload_enable()      //here
>                              devlink_reload_enable()
> 
> Before Thread1's devlink_reload_enable(), the devlink is already allowed
> to execute reload because Thread0 allows it. devlink reload disable/enable
> variable type is bool. So the above case would exist.
> So, disable/enable should be executed atomically.
> In order to do that, a new lock is used.
> 
> Test commands:
>     modprobe netdevsim
>     echo 1 > /sys/bus/netdevsim/new_device
> 
>     while :
>     do
>         echo 1 > /sys/devices/netdevsim1/new_port &
>         echo 1 > /sys/devices/netdevsim1/del_port &
>         devlink dev reload netdevsim/netdevsim1 &
>     done
> 
> Splat looks like:
> [ 1067.313531][ T1480] kernel BUG at lib/list_debug.c:53!
> [ 1067.314519][ T1480] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
> [ 1067.315948][ T1480] CPU: 3 PID: 1480 Comm: bash Tainted: G        W         5.5.0-rc6+ #318
> [ 1067.326082][ T1480] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> [ 1067.356308][ T1480] RIP: 0010:__list_del_entry_valid+0xe6/0x150
> [ 1067.357006][ T1480] Code: 89 ea 48 c7 c7 a0 64 1e 9f e8 7f 5b 4d ff 0f 0b 48 c7 c7 00 65 1e 9f e8 71 5b 4d ff 4
> [ 1067.395359][ T1480] RSP: 0018:ffff8880a316fb58 EFLAGS: 00010282
> [ 1067.396016][ T1480] RAX: 0000000000000054 RBX: ffff8880c0e76718 RCX: 0000000000000000
> [ 1067.402370][ T1480] RDX: 0000000000000054 RSI: 0000000000000008 RDI: ffffed101462df61
> [ 1067.430844][ T1480] RBP: ffff8880a31bfca0 R08: ffffed101b5404f9 R09: ffffed101b5404f9
> [ 1067.432165][ T1480] R10: 0000000000000001 R11: ffffed101b5404f8 R12: ffff8880a316fcb0
> [ 1067.433526][ T1480] R13: ffff8880a310d440 R14: ffffffffa117a7c0 R15: ffff8880c0e766c0
> [ 1067.435818][ T1480] FS:  00007f001c026740(0000) GS:ffff8880da800000(0000) knlGS:0000000000000000
> [ 1067.441677][ T1480] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1067.451305][ T1480] CR2: 00007f001afb7180 CR3: 00000000a3170003 CR4: 00000000000606e0
> [ 1067.453416][ T1480] Call Trace:
> [ 1067.453832][ T1480]  mutex_remove_waiter+0x101/0x520
> [ 1067.455949][ T1480]  __mutex_lock+0xac7/0x14b0
> [ 1067.456880][ T1480]  ? nsim_dev_port_add+0x50/0x150 [netdevsim]
> [ 1067.458946][ T1480]  ? mutex_lock_io_nested+0x1380/0x1380
> [ 1067.460614][ T1480]  ? _parse_integer+0xf0/0xf0
> [ 1067.472498][ T1480]  ? kstrtouint+0x86/0x110
> [ 1067.473327][ T1480]  ? nsim_dev_port_add+0x50/0x150 [netdevsim]
> [ 1067.474187][ T1480]  nsim_dev_port_add+0x50/0x150 [netdevsim]
> [ 1067.474980][ T1480]  new_port_store+0xc4/0xf0 [netdevsim]
> [ 1067.475717][ T1480]  ? del_port_store+0xf0/0xf0 [netdevsim]
> [ 1067.476478][ T1480]  ? sysfs_kf_write+0x3b/0x180
> [ 1067.477106][ T1480]  ? sysfs_file_ops+0x160/0x160
> [ 1067.477744][ T1480]  kernfs_fop_write+0x276/0x410
> [ ... ]
> 
> Fixes: 4418f862d675 ("netdevsim: implement support for devlink region and snapshots")
> Fixes: 75ba029f3c07 ("netdevsim: implement proper devlink reload")
> Signed-off-by: Taehee Yoo <ap420073@...il.com>
> ---
>  drivers/net/netdevsim/bus.c       | 21 ++++++++++++++++++++-
>  drivers/net/netdevsim/dev.c       | 14 ++++++++++++++
>  drivers/net/netdevsim/netdevsim.h |  4 ++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> index a3205fd73c8f..b1aed37a0574 100644
> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -16,7 +16,7 @@
>  
>  static DEFINE_IDA(nsim_bus_dev_ids);
>  static LIST_HEAD(nsim_bus_dev_list);
> -static DEFINE_MUTEX(nsim_bus_dev_ops_lock);
> +DEFINE_MUTEX(nsim_bus_dev_ops_lock);
>  static bool enable;
>  
>  static struct nsim_bus_dev *to_nsim_bus_dev(struct device *dev)
> @@ -97,6 +97,8 @@ new_port_store(struct device *dev, struct device_attribute *attr,
>  	       const char *buf, size_t count)
>  {
>  	struct nsim_bus_dev *nsim_bus_dev = to_nsim_bus_dev(dev);
> +	struct nsim_dev *nsim_dev = dev_get_drvdata(dev);
> +	struct devlink *devlink;
>  	unsigned int port_index;
>  	int ret;
>  
> @@ -105,7 +107,14 @@ new_port_store(struct device *dev, struct device_attribute *attr,
>  	ret = kstrtouint(buf, 0, &port_index);
>  	if (ret)
>  		return ret;
> +
> +	devlink = priv_to_devlink(nsim_dev);
> +
> +	mutex_lock(&nsim_bus_dev->port_ops_lock);
> +	devlink_reload_disable(devlink);
>  	ret = nsim_dev_port_add(nsim_bus_dev, port_index);
> +	devlink_reload_enable(devlink);
> +	mutex_unlock(&nsim_bus_dev->port_ops_lock);
>  	return ret ? ret : count;
>  }
>  
> @@ -116,6 +125,8 @@ del_port_store(struct device *dev, struct device_attribute *attr,
>  	       const char *buf, size_t count)
>  {
>  	struct nsim_bus_dev *nsim_bus_dev = to_nsim_bus_dev(dev);
> +	struct nsim_dev *nsim_dev = dev_get_drvdata(dev);
> +	struct devlink *devlink;
>  	unsigned int port_index;
>  	int ret;
>  
> @@ -124,7 +135,14 @@ del_port_store(struct device *dev, struct device_attribute *attr,
>  	ret = kstrtouint(buf, 0, &port_index);
>  	if (ret)
>  		return ret;
> +
> +	devlink = priv_to_devlink(nsim_dev);
> +
> +	mutex_lock(&nsim_bus_dev->port_ops_lock);
> +	devlink_reload_disable(devlink);
>  	ret = nsim_dev_port_del(nsim_bus_dev, port_index);
> +	devlink_reload_enable(devlink);
> +	mutex_unlock(&nsim_bus_dev->port_ops_lock);
>  	return ret ? ret : count;
>  }
>  
> @@ -301,6 +319,7 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count)
>  	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;
> +	mutex_init(&nsim_bus_dev->port_ops_lock);
>  
>  	err = device_register(&nsim_bus_dev->dev);
>  	if (err)

Disabling reload around port add/del makes perfect sense

> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index 4b39aba2e9c4..0dfaf999e8db 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -43,6 +43,8 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>  					    size_t count, loff_t *ppos)
>  {
>  	struct nsim_dev *nsim_dev = file->private_data;
> +	struct nsim_bus_dev *nsim_bus_dev;
> +	struct devlink *devlink;
>  	void *dummy_data;
>  	int err;
>  	u32 id;
> @@ -51,11 +53,23 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
>  	if (!dummy_data)
>  		return -ENOMEM;
>  
> +	devlink = priv_to_devlink(nsim_dev);
> +	nsim_bus_dev = nsim_dev->nsim_bus_dev;
> +
>  	get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
>  
> +	mutex_lock(&nsim_bus_dev_ops_lock);

Not sure why we have to lock the big lock here?

> +	mutex_lock(&nsim_bus_dev->port_ops_lock);
> +	devlink_reload_disable(devlink);

Or the port lock, or disable reload - the reload path unregisters and
re-registers the snapshot, so snapshot can't be taken during reload, 
I don't think.

>  	id = devlink_region_snapshot_id_get(priv_to_devlink(nsim_dev));
>  	err = devlink_region_snapshot_create(nsim_dev->dummy_region,
>  					     dummy_data, id, kfree);
> +
> +	devlink_reload_enable(devlink);
> +	mutex_unlock(&nsim_bus_dev->port_ops_lock);
> +	mutex_unlock(&nsim_bus_dev_ops_lock);
> +
>  	if (err) {
>  		pr_err("Failed to create region snapshot\n");
>  		kfree(dummy_data);
> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index ea3931391ce2..a8b6c9e6f79f 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -74,6 +74,8 @@ struct netdevsim {
>  	struct nsim_ipsec ipsec;
>  };
>  
> +extern struct mutex nsim_bus_dev_ops_lock;
> +
>  struct netdevsim *
>  nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port);
>  void nsim_destroy(struct netdevsim *ns);
> @@ -240,6 +242,8 @@ struct nsim_bus_dev {
>  				  */
>  	unsigned int num_vfs;
>  	struct nsim_vf_config *vfconfigs;
> +	/* Lock for new_port() and del_port() to disable devlink reload */

It'd be good to have the comment point out which fields the lock
protects, rather than code.

> +	struct mutex port_ops_lock;
>  	bool init;
>  };
>  

Powered by blists - more mailing lists