All of lore.kernel.org
 help / color / mirror / Atom feed
From: Taehee Yoo <ap420073@gmail.com>
To: davem@davemloft.net, jakub.kicinski@netronome.com,
	netdev@vger.kernel.org
Cc: ap420073@gmail.com
Subject: [PATCH net 1/5] netdevsim: fix a race condition in netdevsim operations
Date: Sat, 11 Jan 2020 16:36:55 +0000	[thread overview]
Message-ID: <20200111163655.4087-1-ap420073@gmail.com> (raw)

netdevsim basic operations are called with sysfs.

Create netdevsim device:
echo "1 1" > /sys/bus/netdevsim/new_device
Delete netdevsim device:
echo 1 > /sys/bus/netdevsim/del_device
Create netdevsim port:
echo 4 > /sys/devices/netdevsim1/new_port
Delete netdevsim port:
echo 4 > /sys/devices/netdevsim1/del_port
Set sriov number:
echo 4 > /sys/devices/netdevsim1/sriov_numvfs

These operations use the same resource so they should acquire a lock for
the whole resource not only for a list.

Test commands:
    #SHELL1
    modprobe netdevsim
    while :
    do
        echo "1 1" > /sys/bus/netdevsim/new_device
        echo "1 1" > /sys/bus/netdevsim/del_device
    done

    #SHELL2
    while :
    do
        echo 1 > /sys/devices/netdevsim1/new_port
        echo 1 > /sys/devices/netdevsim1/del_port
    done

Splat looks like:
[  151.623634][ T1165] kasan: CONFIG_KASAN_INLINE enabled
[  151.626503][ T1165] kasan: GPF could be caused by NULL-ptr deref or user memory access
[  151.627862][ T1165] general protection fault: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
[  151.630700][ T1165] CPU: 3 PID: 1165 Comm: bash Not tainted 5.5.0-rc5+ #270
[  151.633339][ T1165] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[  151.635758][ T1165] RIP: 0010:__mutex_lock+0x10a/0x14b0
[  151.636713][ T1165] Code: 08 84 d2 0f 85 7f 12 00 00 44 8b 0d 20 66 67 02 45 85 c9 75 29 49 8d 7f 68 48 b8 00 f
[  151.641725][ T1165] RSP: 0018:ffff8880b17cfbb0 EFLAGS: 00010206
[  151.642547][ T1165] RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000000000
[  151.643636][ T1165] RDX: 0000000000000021 RSI: ffffffffbb922c00 RDI: 0000000000000108
[  151.645415][ T1165] RBP: ffff8880b17cfd30 R08: ffffffffc0392ad0 R09: 0000000000000000
[  151.646803][ T1165] R10: ffff8880b17cfd50 R11: ffff8880a9225440 R12: 0000000000000000
[  151.647846][ T1165] R13: dffffc0000000000 R14: ffffffffbd16e7c0 R15: 00000000000000a0
[  151.651979][ T1165] FS:  00007f9731c8e740(0000) GS:ffff8880da800000(0000) knlGS:0000000000000000
[  151.655905][ T1165] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  151.656828][ T1165] CR2: 000055be472e1140 CR3: 00000000b1e14005 CR4: 00000000000606e0
[  151.657906][ T1165] Call Trace:
[  151.658346][ T1165]  ? nsim_dev_port_add+0x50/0x150 [netdevsim]
[  151.659146][ T1165]  ? mutex_lock_io_nested+0x1380/0x1380
[  151.659902][ T1165]  ? _kstrtoull+0x76/0x160
[  151.660480][ T1165]  ? _parse_integer+0xf0/0xf0
[  151.661097][ T1165]  ? kernfs_fop_write+0x1cf/0x410
[  151.661751][ T1165]  ? sysfs_file_ops+0x160/0x160
[  151.662389][ T1165]  ? kstrtouint+0x86/0x110
[  151.662972][ T1165]  ? nsim_dev_port_add+0x50/0x150 [netdevsim]
[  151.663768][ T1165]  nsim_dev_port_add+0x50/0x150 [netdevsim]
[ ... ]

In this patch, __init and __exit function also acquire a lock.
operations could be called while __init and __exit functions are
processing. If so, uninitialized or freed resources could be used.
So, __init() and __exit function also need lock.

Fixes: 83c9e13aa39a ("netdevsim: add software driver for testing offloads")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 drivers/net/netdevsim/bus.c       | 64 ++++++++++++++++++++++++-------
 drivers/net/netdevsim/dev.c       | 20 +++++++++-
 drivers/net/netdevsim/netdevsim.h |  2 +
 3 files changed, 71 insertions(+), 15 deletions(-)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index 6aeed0c600f8..b40e4e70995d 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -16,7 +16,8 @@
 
 static DEFINE_IDA(nsim_bus_dev_ids);
 static LIST_HEAD(nsim_bus_dev_list);
-static DEFINE_MUTEX(nsim_bus_dev_list_lock);
+/* mutex lock for netdevsim operations */
+DEFINE_MUTEX(nsim_bus_dev_ops_lock);
 
 static struct nsim_bus_dev *to_nsim_bus_dev(struct device *dev)
 {
@@ -51,9 +52,14 @@ nsim_bus_dev_numvfs_store(struct device *dev, struct device_attribute *attr,
 	unsigned int num_vfs;
 	int ret;
 
+	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
+		return -EBUSY;
+
 	ret = kstrtouint(buf, 0, &num_vfs);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		return ret;
+	}
 
 	rtnl_lock();
 	if (nsim_bus_dev->num_vfs == num_vfs)
@@ -74,6 +80,7 @@ nsim_bus_dev_numvfs_store(struct device *dev, struct device_attribute *attr,
 	ret = count;
 exit_unlock:
 	rtnl_unlock();
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 
 	return ret;
 }
@@ -83,8 +90,13 @@ nsim_bus_dev_numvfs_show(struct device *dev,
 			 struct device_attribute *attr, char *buf)
 {
 	struct nsim_bus_dev *nsim_bus_dev = to_nsim_bus_dev(dev);
+	int ret;
 
-	return sprintf(buf, "%u\n", nsim_bus_dev->num_vfs);
+	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
+		return -EBUSY;
+	ret = sprintf(buf, "%u\n", nsim_bus_dev->num_vfs);
+	mutex_unlock(&nsim_bus_dev_ops_lock);
+	return ret;
 }
 
 static struct device_attribute nsim_bus_dev_numvfs_attr =
@@ -99,10 +111,15 @@ new_port_store(struct device *dev, struct device_attribute *attr,
 	unsigned int port_index;
 	int ret;
 
+	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
+		return -EBUSY;
 	ret = kstrtouint(buf, 0, &port_index);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		return ret;
+	}
 	ret = nsim_dev_port_add(nsim_bus_dev, port_index);
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 	return ret ? ret : count;
 }
 
@@ -116,10 +133,17 @@ del_port_store(struct device *dev, struct device_attribute *attr,
 	unsigned int port_index;
 	int ret;
 
+	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
+		return -EBUSY;
+
 	ret = kstrtouint(buf, 0, &port_index);
-	if (ret)
+	if (ret) {
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		return ret;
+	}
+
 	ret = nsim_dev_port_del(nsim_bus_dev, port_index);
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 	return ret ? ret : count;
 }
 
@@ -179,13 +203,17 @@ new_device_store(struct bus_type *bus, const char *buf, size_t count)
 		pr_err("Format for adding new device is \"id port_count\" (uint uint).\n");
 		return -EINVAL;
 	}
+
+	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
+		return -EBUSY;
 	nsim_bus_dev = nsim_bus_dev_new(id, port_count);
-	if (IS_ERR(nsim_bus_dev))
+	if (IS_ERR(nsim_bus_dev)) {
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		return PTR_ERR(nsim_bus_dev);
+	}
 
-	mutex_lock(&nsim_bus_dev_list_lock);
 	list_add_tail(&nsim_bus_dev->list, &nsim_bus_dev_list);
-	mutex_unlock(&nsim_bus_dev_list_lock);
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 
 	return count;
 }
@@ -214,7 +242,8 @@ del_device_store(struct bus_type *bus, const char *buf, size_t count)
 	}
 
 	err = -ENOENT;
-	mutex_lock(&nsim_bus_dev_list_lock);
+	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
+		return -EBUSY;
 	list_for_each_entry_safe(nsim_bus_dev, tmp, &nsim_bus_dev_list, list) {
 		if (nsim_bus_dev->dev.id != id)
 			continue;
@@ -223,7 +252,7 @@ del_device_store(struct bus_type *bus, const char *buf, size_t count)
 		err = 0;
 		break;
 	}
-	mutex_unlock(&nsim_bus_dev_list_lock);
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 	return !err ? count : err;
 }
 static BUS_ATTR_WO(del_device);
@@ -314,12 +343,19 @@ int nsim_bus_init(void)
 {
 	int err;
 
+	mutex_lock(&nsim_bus_dev_ops_lock);
 	err = bus_register(&nsim_bus);
-	if (err)
+	if (err) {
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		return err;
+	}
 	err = driver_register(&nsim_driver);
-	if (err)
+	if (err) {
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		goto err_bus_unregister;
+	}
+
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 	return 0;
 
 err_bus_unregister:
@@ -331,12 +367,12 @@ void nsim_bus_exit(void)
 {
 	struct nsim_bus_dev *nsim_bus_dev, *tmp;
 
-	mutex_lock(&nsim_bus_dev_list_lock);
+	mutex_lock(&nsim_bus_dev_ops_lock);
 	list_for_each_entry_safe(nsim_bus_dev, tmp, &nsim_bus_dev_list, list) {
 		list_del(&nsim_bus_dev->list);
 		nsim_bus_dev_del(nsim_bus_dev);
 	}
-	mutex_unlock(&nsim_bus_dev_list_lock);
 	driver_unregister(&nsim_driver);
 	bus_unregister(&nsim_bus);
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 }
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 059711edfc61..634eb5cdcbbe 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -43,13 +43,24 @@ 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 devlink *devlink;
 	void *dummy_data;
 	int err;
 	u32 id;
 
+	devlink = priv_to_devlink(nsim_dev);
+
+	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
+		return -EBUSY;
+
+	devlink_reload_disable(devlink);
+
 	dummy_data = kmalloc(NSIM_DEV_DUMMY_REGION_SIZE, GFP_KERNEL);
-	if (!dummy_data)
+	if (!dummy_data) {
+		devlink_reload_enable(devlink);
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		return -ENOMEM;
+	}
 
 	get_random_bytes(dummy_data, NSIM_DEV_DUMMY_REGION_SIZE);
 
@@ -59,9 +70,13 @@ static ssize_t nsim_dev_take_snapshot_write(struct file *file,
 	if (err) {
 		pr_err("Failed to create region snapshot\n");
 		kfree(dummy_data);
+		devlink_reload_enable(devlink);
+		mutex_unlock(&nsim_bus_dev_ops_lock);
 		return err;
 	}
 
+	devlink_reload_enable(devlink);
+	mutex_unlock(&nsim_bus_dev_ops_lock);
 	return count;
 }
 
@@ -909,9 +924,12 @@ int nsim_dev_port_del(struct nsim_bus_dev *nsim_bus_dev,
 		      unsigned int port_index)
 {
 	struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
+	struct devlink *devlink = priv_to_devlink(nsim_dev);
 	struct nsim_dev_port *nsim_dev_port;
 	int err = 0;
 
+	devlink_reload_disable(devlink);
+
 	mutex_lock(&nsim_dev->port_list_lock);
 	nsim_dev_port = __nsim_dev_port_lookup(nsim_dev, port_index);
 	if (!nsim_dev_port)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 94df795ef4d3..6a7cfd2319df 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -30,6 +30,8 @@
 #define NSIM_IPSEC_MAX_SA_COUNT		33
 #define NSIM_IPSEC_VALID		BIT(31)
 
+extern struct mutex nsim_bus_dev_ops_lock;
+
 struct nsim_sa {
 	struct xfrm_state *xs;
 	__be32 ipaddr[4];
-- 
2.17.1


             reply	other threads:[~2020-01-11 16:37 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-11 16:36 Taehee Yoo [this message]
2020-01-12 14:19 ` [PATCH net 1/5] netdevsim: fix a race condition in netdevsim operations Jakub Kicinski
2020-01-14 15:26   ` Taehee Yoo
2020-01-15 14:16     ` Jakub Kicinski
2020-01-24  5:16       ` Taehee Yoo
2020-01-24 13:53         ` Jakub Kicinski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200111163655.4087-1-ap420073@gmail.com \
    --to=ap420073@gmail.com \
    --cc=davem@davemloft.net \
    --cc=jakub.kicinski@netronome.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.