linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] fix races between nbd setup and module removal
@ 2021-09-04 12:25 Hou Tao
  2021-09-04 12:25 ` [PATCH v2 1/3] nbd: use pr_err to output error message Hou Tao
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Hou Tao @ 2021-09-04 12:25 UTC (permalink / raw)
  To: Josef Bacik, Jens Axboe, linux-block; +Cc: nbd, hch, houtao1

Hi,

The patch series aims to fix the races between nbd setup and module
removal which may lead to oops. Patch #1 is just replacing
printk(KERN_ERR "nbd: ...") by pr_err("...") which makes it easier
to add error message in patch #3. Patch #2 serializes the concurrently
calling of nbd_genl_connect() and nbd_cleanup(), and patch #3 fixes race
between nbd_alloc_config() and nbd_cleanup().

Any comments are welcome.

Regards,
Tao

ChangeLog:
v2:
  * add a new patch "use pr_err to output error message"
  * add the missing error message in patch 3.

v1: https://www.spinics.net/lists/linux-block/msg72995.html

Hou Tao (3):
  nbd: use pr_err to output error message
  nbd: call genl_unregister_family() first in nbd_cleanup()
  nbd: fix race between nbd_alloc_config() and module removal

 drivers/block/nbd.c | 70 ++++++++++++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 26 deletions(-)

-- 
2.29.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] nbd: use pr_err to output error message
  2021-09-04 12:25 [PATCH v2 0/3] fix races between nbd setup and module removal Hou Tao
@ 2021-09-04 12:25 ` Hou Tao
  2021-09-06  9:27   ` Christoph Hellwig
  2021-09-04 12:25 ` [PATCH v2 2/3] nbd: call genl_unregister_family() first in nbd_cleanup() Hou Tao
  2021-09-04 12:25 ` [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal Hou Tao
  2 siblings, 1 reply; 15+ messages in thread
From: Hou Tao @ 2021-09-04 12:25 UTC (permalink / raw)
  To: Josef Bacik, Jens Axboe, linux-block; +Cc: nbd, hch, houtao1

Instead of using the long printk(KERN_ERR "nbd: ...") to
output error message, defining pr_fmt and using
the short pr_err("") to do that. The replacemen is done
by using the following command:

  sed -i 's/printk(KERN_ERR "nbd: /pr_err("/g' \
		  drivers/block/nbd.c

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/block/nbd.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5170a630778d..ef94f62895d6 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -44,6 +44,9 @@
 #include <linux/nbd-netlink.h>
 #include <net/genetlink.h>
 
+#undef pr_fmt
+#define pr_fmt(fmt) "nbd: " fmt
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/nbd.h>
 
@@ -1854,11 +1857,11 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 	if (info->attrs[NBD_ATTR_INDEX])
 		index = nla_get_u32(info->attrs[NBD_ATTR_INDEX]);
 	if (!info->attrs[NBD_ATTR_SOCKETS]) {
-		printk(KERN_ERR "nbd: must specify at least one socket\n");
+		pr_err("must specify at least one socket\n");
 		return -EINVAL;
 	}
 	if (!info->attrs[NBD_ATTR_SIZE_BYTES]) {
-		printk(KERN_ERR "nbd: must specify a size in bytes for the device\n");
+		pr_err("must specify a size in bytes for the device\n");
 		return -EINVAL;
 	}
 again:
@@ -1894,7 +1897,7 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 		nbd_put(nbd);
 		if (index == -1)
 			goto again;
-		printk(KERN_ERR "nbd: nbd%d already in use\n", index);
+		pr_err("nbd%d already in use\n", index);
 		return -EBUSY;
 	}
 	if (WARN_ON(nbd->config)) {
@@ -1906,7 +1909,7 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 	if (!nbd->config) {
 		mutex_unlock(&nbd->config_lock);
 		nbd_put(nbd);
-		printk(KERN_ERR "nbd: couldn't allocate config\n");
+		pr_err("nbd: couldn't allocate config\n");
 		return -ENOMEM;
 	}
 	refcount_set(&nbd->config_refs, 1);
@@ -1961,7 +1964,7 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 			struct nlattr *socks[NBD_SOCK_MAX+1];
 
 			if (nla_type(attr) != NBD_SOCK_ITEM) {
-				printk(KERN_ERR "nbd: socks must be embedded in a SOCK_ITEM attr\n");
+				pr_err("socks must be embedded in a SOCK_ITEM attr\n");
 				ret = -EINVAL;
 				goto out;
 			}
@@ -1970,7 +1973,7 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 							  nbd_sock_policy,
 							  info->extack);
 			if (ret != 0) {
-				printk(KERN_ERR "nbd: error processing sock list\n");
+				pr_err("error processing sock list\n");
 				ret = -EINVAL;
 				goto out;
 			}
@@ -2044,7 +2047,7 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
 		return -EPERM;
 
 	if (!info->attrs[NBD_ATTR_INDEX]) {
-		printk(KERN_ERR "nbd: must specify an index to disconnect\n");
+		pr_err("must specify an index to disconnect\n");
 		return -EINVAL;
 	}
 	index = nla_get_u32(info->attrs[NBD_ATTR_INDEX]);
@@ -2052,13 +2055,13 @@ static int nbd_genl_disconnect(struct sk_buff *skb, struct genl_info *info)
 	nbd = idr_find(&nbd_index_idr, index);
 	if (!nbd) {
 		mutex_unlock(&nbd_index_mutex);
-		printk(KERN_ERR "nbd: couldn't find device at index %d\n",
+		pr_err("couldn't find device at index %d\n",
 		       index);
 		return -EINVAL;
 	}
 	if (!refcount_inc_not_zero(&nbd->refs)) {
 		mutex_unlock(&nbd_index_mutex);
-		printk(KERN_ERR "nbd: device at index %d is going down\n",
+		pr_err("device at index %d is going down\n",
 		       index);
 		return -EINVAL;
 	}
@@ -2084,7 +2087,7 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
 		return -EPERM;
 
 	if (!info->attrs[NBD_ATTR_INDEX]) {
-		printk(KERN_ERR "nbd: must specify a device to reconfigure\n");
+		pr_err("must specify a device to reconfigure\n");
 		return -EINVAL;
 	}
 	index = nla_get_u32(info->attrs[NBD_ATTR_INDEX]);
@@ -2092,7 +2095,7 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
 	nbd = idr_find(&nbd_index_idr, index);
 	if (!nbd) {
 		mutex_unlock(&nbd_index_mutex);
-		printk(KERN_ERR "nbd: couldn't find a device at index %d\n",
+		pr_err("couldn't find a device at index %d\n",
 		       index);
 		return -EINVAL;
 	}
@@ -2114,7 +2117,7 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
 	}
 	if (!refcount_inc_not_zero(&nbd->refs)) {
 		mutex_unlock(&nbd_index_mutex);
-		printk(KERN_ERR "nbd: device at index %d is going down\n",
+		pr_err("device at index %d is going down\n",
 		       index);
 		return -EINVAL;
 	}
@@ -2179,7 +2182,7 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
 			struct nlattr *socks[NBD_SOCK_MAX+1];
 
 			if (nla_type(attr) != NBD_SOCK_ITEM) {
-				printk(KERN_ERR "nbd: socks must be embedded in a SOCK_ITEM attr\n");
+				pr_err("socks must be embedded in a SOCK_ITEM attr\n");
 				ret = -EINVAL;
 				goto out;
 			}
@@ -2188,7 +2191,7 @@ static int nbd_genl_reconfigure(struct sk_buff *skb, struct genl_info *info)
 							  nbd_sock_policy,
 							  info->extack);
 			if (ret != 0) {
-				printk(KERN_ERR "nbd: error processing sock list\n");
+				pr_err("error processing sock list\n");
 				ret = -EINVAL;
 				goto out;
 			}
@@ -2405,7 +2408,7 @@ static int __init nbd_init(void)
 	BUILD_BUG_ON(sizeof(struct nbd_request) != 28);
 
 	if (max_part < 0) {
-		printk(KERN_ERR "nbd: max_part must be >= 0\n");
+		pr_err("max_part must be >= 0\n");
 		return -EINVAL;
 	}
 
@@ -2478,7 +2481,7 @@ static void __exit nbd_cleanup(void)
 		nbd = list_first_entry(&del_list, struct nbd_device, list);
 		list_del_init(&nbd->list);
 		if (refcount_read(&nbd->refs) != 1)
-			printk(KERN_ERR "nbd: possibly leaking a device\n");
+			pr_err("possibly leaking a device\n");
 		nbd_put(nbd);
 	}
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] nbd: call genl_unregister_family() first in nbd_cleanup()
  2021-09-04 12:25 [PATCH v2 0/3] fix races between nbd setup and module removal Hou Tao
  2021-09-04 12:25 ` [PATCH v2 1/3] nbd: use pr_err to output error message Hou Tao
@ 2021-09-04 12:25 ` Hou Tao
  2021-09-06  9:27   ` Christoph Hellwig
  2021-09-04 12:25 ` [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal Hou Tao
  2 siblings, 1 reply; 15+ messages in thread
From: Hou Tao @ 2021-09-04 12:25 UTC (permalink / raw)
  To: Josef Bacik, Jens Axboe, linux-block; +Cc: nbd, hch, houtao1

Otherwise there may be race between module removal and the handling of
netlink command, which can lead to the oops as shown below:

  BUG: kernel NULL pointer dereference, address: 0000000000000098
  Oops: 0002 [#1] SMP PTI
  CPU: 1 PID: 31299 Comm: nbd-client Tainted: G            E     5.14.0-rc4
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
  RIP: 0010:down_write+0x1a/0x50
  Call Trace:
   start_creating+0x89/0x130
   debugfs_create_dir+0x1b/0x130
   nbd_start_device+0x13d/0x390 [nbd]
   nbd_genl_connect+0x42f/0x748 [nbd]
   genl_family_rcv_msg_doit.isra.0+0xec/0x150
   genl_rcv_msg+0xe5/0x1e0
   netlink_rcv_skb+0x55/0x100
   genl_rcv+0x29/0x40
   netlink_unicast+0x1a8/0x250
   netlink_sendmsg+0x21b/0x430
   ____sys_sendmsg+0x2a4/0x2d0
   ___sys_sendmsg+0x81/0xc0
   __sys_sendmsg+0x62/0xb0
   __x64_sys_sendmsg+0x1f/0x30
   do_syscall_64+0x3b/0xc0
   entry_SYSCALL_64_after_hwframe+0x44/0xae
  Modules linked in: nbd(E-)

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
v2:
 * update comment and commit message (suggested by eblake@redhat.com)
---
 drivers/block/nbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index ef94f62895d6..cedd3648e1a7 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2471,6 +2471,12 @@ static void __exit nbd_cleanup(void)
 	struct nbd_device *nbd;
 	LIST_HEAD(del_list);
 
+	/*
+	 * Unregister netlink interface prior to waiting
+	 * for the completion of netlink commands.
+	 */
+	genl_unregister_family(&nbd_genl_family);
+
 	nbd_dbg_close();
 
 	mutex_lock(&nbd_index_mutex);
@@ -2489,7 +2495,6 @@ static void __exit nbd_cleanup(void)
 	destroy_workqueue(nbd_del_wq);
 
 	idr_destroy(&nbd_index_idr);
-	genl_unregister_family(&nbd_genl_family);
 	unregister_blkdev(NBD_MAJOR, "nbd");
 }
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-04 12:25 [PATCH v2 0/3] fix races between nbd setup and module removal Hou Tao
  2021-09-04 12:25 ` [PATCH v2 1/3] nbd: use pr_err to output error message Hou Tao
  2021-09-04 12:25 ` [PATCH v2 2/3] nbd: call genl_unregister_family() first in nbd_cleanup() Hou Tao
@ 2021-09-04 12:25 ` Hou Tao
  2021-09-06  9:30   ` Christoph Hellwig
  2 siblings, 1 reply; 15+ messages in thread
From: Hou Tao @ 2021-09-04 12:25 UTC (permalink / raw)
  To: Josef Bacik, Jens Axboe, linux-block; +Cc: nbd, hch, houtao1

When nbd module is being removing, nbd_alloc_config() may be
called concurrently by nbd_genl_connect(), although try_module_get()
will return false, but nbd_alloc_config() doesn't handle it.

The race may lead to the leak of nbd_config and its related
resources (e.g, recv_workq) and oops in nbd_read_stat() due
to the unload of nbd module as shown below:

  BUG: kernel NULL pointer dereference, address: 0000000000000040
  Oops: 0000 [#1] SMP PTI
  CPU: 5 PID: 13840 Comm: kworker/u17:33 Not tainted 5.14.0+ #1
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
  Workqueue: knbd16-recv recv_work [nbd]
  RIP: 0010:nbd_read_stat.cold+0x130/0x1a4 [nbd]
  Call Trace:
   recv_work+0x3b/0xb0 [nbd]
   process_one_work+0x1ed/0x390
   worker_thread+0x4a/0x3d0
   kthread+0x12a/0x150
   ret_from_fork+0x22/0x30

Fixing it by checking the return value of try_module_get()
in nbd_alloc_config(). As nbd_alloc_config() may return ERR_PTR(-ENODEV),
assign nbd->config only when nbd_alloc_config() succeeds to ensure
the value of nbd->config is binary (valid or NULL).

Also adding a debug message to check the reference counter
of nbd_config during module removal.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 drivers/block/nbd.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index cedd3648e1a7..fa6c069b79dc 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1473,15 +1473,20 @@ static struct nbd_config *nbd_alloc_config(void)
 {
 	struct nbd_config *config;
 
+	if (!try_module_get(THIS_MODULE))
+		return ERR_PTR(-ENODEV);
+
 	config = kzalloc(sizeof(struct nbd_config), GFP_NOFS);
-	if (!config)
-		return NULL;
+	if (!config) {
+		module_put(THIS_MODULE);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	atomic_set(&config->recv_threads, 0);
 	init_waitqueue_head(&config->recv_wq);
 	init_waitqueue_head(&config->conn_wait);
 	config->blksize = NBD_DEF_BLKSIZE;
 	atomic_set(&config->live_connections, 0);
-	try_module_get(THIS_MODULE);
 	return config;
 }
 
@@ -1508,12 +1513,13 @@ static int nbd_open(struct block_device *bdev, fmode_t mode)
 			mutex_unlock(&nbd->config_lock);
 			goto out;
 		}
-		config = nbd->config = nbd_alloc_config();
-		if (!config) {
-			ret = -ENOMEM;
+		config = nbd_alloc_config();
+		if (IS_ERR(config)) {
+			ret = PTR_ERR(config);
 			mutex_unlock(&nbd->config_lock);
 			goto out;
 		}
+		nbd->config = config;
 		refcount_set(&nbd->config_refs, 1);
 		refcount_inc(&nbd->refs);
 		mutex_unlock(&nbd->config_lock);
@@ -1905,13 +1911,14 @@ static int nbd_genl_connect(struct sk_buff *skb, struct genl_info *info)
 		nbd_put(nbd);
 		return -EINVAL;
 	}
-	config = nbd->config = nbd_alloc_config();
-	if (!nbd->config) {
+	config = nbd_alloc_config();
+	if (IS_ERR(config)) {
 		mutex_unlock(&nbd->config_lock);
 		nbd_put(nbd);
 		pr_err("nbd: couldn't allocate config\n");
-		return -ENOMEM;
+		return PTR_ERR(config);
 	}
+	nbd->config = config;
 	refcount_set(&nbd->config_refs, 1);
 	set_bit(NBD_RT_BOUND, &config->runtime_flags);
 
@@ -2486,6 +2493,9 @@ static void __exit nbd_cleanup(void)
 	while (!list_empty(&del_list)) {
 		nbd = list_first_entry(&del_list, struct nbd_device, list);
 		list_del_init(&nbd->list);
+		if (refcount_read(&nbd->config_refs))
+			pr_err("possibly leaking nbd_config (ref %d)\n",
+			       refcount_read(&nbd->config_refs));
 		if (refcount_read(&nbd->refs) != 1)
 			pr_err("possibly leaking a device\n");
 		nbd_put(nbd);
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/3] nbd: use pr_err to output error message
  2021-09-04 12:25 ` [PATCH v2 1/3] nbd: use pr_err to output error message Hou Tao
@ 2021-09-06  9:27   ` Christoph Hellwig
  0 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2021-09-06  9:27 UTC (permalink / raw)
  To: Hou Tao; +Cc: Josef Bacik, Jens Axboe, linux-block, nbd, hch

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/3] nbd: call genl_unregister_family() first in nbd_cleanup()
  2021-09-04 12:25 ` [PATCH v2 2/3] nbd: call genl_unregister_family() first in nbd_cleanup() Hou Tao
@ 2021-09-06  9:27   ` Christoph Hellwig
  0 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2021-09-06  9:27 UTC (permalink / raw)
  To: Hou Tao; +Cc: Josef Bacik, Jens Axboe, linux-block, nbd, hch

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-04 12:25 ` [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal Hou Tao
@ 2021-09-06  9:30   ` Christoph Hellwig
  2021-09-06 10:08     ` Hou Tao
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2021-09-06  9:30 UTC (permalink / raw)
  To: Hou Tao; +Cc: Josef Bacik, Jens Axboe, linux-block, nbd, hch

On Sat, Sep 04, 2021 at 08:25:19PM +0800, Hou Tao wrote:
> When nbd module is being removing, nbd_alloc_config() may be
> called concurrently by nbd_genl_connect(), although try_module_get()
> will return false, but nbd_alloc_config() doesn't handle it.
> 
> The race may lead to the leak of nbd_config and its related
> resources (e.g, recv_workq) and oops in nbd_read_stat() due
> to the unload of nbd module as shown below:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000040
>   Oops: 0000 [#1] SMP PTI
>   CPU: 5 PID: 13840 Comm: kworker/u17:33 Not tainted 5.14.0+ #1
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>   Workqueue: knbd16-recv recv_work [nbd]
>   RIP: 0010:nbd_read_stat.cold+0x130/0x1a4 [nbd]
>   Call Trace:
>    recv_work+0x3b/0xb0 [nbd]
>    process_one_work+0x1ed/0x390
>    worker_thread+0x4a/0x3d0
>    kthread+0x12a/0x150
>    ret_from_fork+0x22/0x30
> 
> Fixing it by checking the return value of try_module_get()
> in nbd_alloc_config(). As nbd_alloc_config() may return ERR_PTR(-ENODEV),
> assign nbd->config only when nbd_alloc_config() succeeds to ensure
> the value of nbd->config is binary (valid or NULL).
> 
> Also adding a debug message to check the reference counter
> of nbd_config during module removal.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---
>  drivers/block/nbd.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index cedd3648e1a7..fa6c069b79dc 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1473,15 +1473,20 @@ static struct nbd_config *nbd_alloc_config(void)
>  {
>  	struct nbd_config *config;
>  
> +	if (!try_module_get(THIS_MODULE))
> +		return ERR_PTR(-ENODEV);

try_module_get(THIS_MODULE) is an indicator for an unsafe pattern.  If
we don't already have a reference it could never close the race.

Looking at the callers:

 - nbd_open like all block device operations must have a reference
   already.
 - for nbd_genl_connect I'm not an expert, but given that struct
   nbd_genl_family has a module member I suspect the networkinh
   code already takes a reference.

So this should be able to use __module_get.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-06  9:30   ` Christoph Hellwig
@ 2021-09-06 10:08     ` Hou Tao
  2021-09-06 10:25       ` Christoph Hellwig
  0 siblings, 1 reply; 15+ messages in thread
From: Hou Tao @ 2021-09-06 10:08 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Josef Bacik, Jens Axboe, linux-block, nbd

Hi,

On 9/6/2021 5:30 PM, Christoph Hellwig wrote:
> On Sat, Sep 04, 2021 at 08:25:19PM +0800, Hou Tao wrote:
>> When nbd module is being removing, nbd_alloc_config() may be
>> called concurrently by nbd_genl_connect(), although try_module_get()
>> will return false, but nbd_alloc_config() doesn't handle it.
>>
>> The race may lead to the leak of nbd_config and its related
>> resources (e.g, recv_workq) and oops in nbd_read_stat() due
>> to the unload of nbd module as shown below:
>>
>>   BUG: kernel NULL pointer dereference, address: 0000000000000040
>>   Oops: 0000 [#1] SMP PTI
>>   CPU: 5 PID: 13840 Comm: kworker/u17:33 Not tainted 5.14.0+ #1
>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
>>   Workqueue: knbd16-recv recv_work [nbd]
>>   RIP: 0010:nbd_read_stat.cold+0x130/0x1a4 [nbd]
>>   Call Trace:
>>    recv_work+0x3b/0xb0 [nbd]
>>    process_one_work+0x1ed/0x390
>>    worker_thread+0x4a/0x3d0
>>    kthread+0x12a/0x150
>>    ret_from_fork+0x22/0x30
>>
>> Fixing it by checking the return value of try_module_get()
>> in nbd_alloc_config(). As nbd_alloc_config() may return ERR_PTR(-ENODEV),
>> assign nbd->config only when nbd_alloc_config() succeeds to ensure
>> the value of nbd->config is binary (valid or NULL).
>>
>> Also adding a debug message to check the reference counter
>> of nbd_config during module removal.
>>
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
>>  drivers/block/nbd.c | 28 +++++++++++++++++++---------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> index cedd3648e1a7..fa6c069b79dc 100644
>> --- a/drivers/block/nbd.c
>> +++ b/drivers/block/nbd.c
>> @@ -1473,15 +1473,20 @@ static struct nbd_config *nbd_alloc_config(void)
>>  {
>>  	struct nbd_config *config;
>>  
>> +	if (!try_module_get(THIS_MODULE))
>> +		return ERR_PTR(-ENODEV);
> try_module_get(THIS_MODULE) is an indicator for an unsafe pattern.  If
> we don't already have a reference it could never close the race.
>
> Looking at the callers:
>
>  - nbd_open like all block device operations must have a reference
>    already.
Yes. nbd_open() has already taken a reference in dentry_open().
>  - for nbd_genl_connect I'm not an expert, but given that struct
>    nbd_genl_family has a module member I suspect the networkinh
>    code already takes a reference.

That was my original though, but the fact is netlink code doesn't take a module reference

in genl_family_rcv_msg_doit() and netlink uses genl_lock_all() to serialize between module removal

and nbd_connect_genl_ops calling, so I think use try_module_get() is OK here.

Regards,

Tao


> So this should be able to use __module_get.

> .

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-06 10:08     ` Hou Tao
@ 2021-09-06 10:25       ` Christoph Hellwig
  2021-09-07  3:04         ` Hou Tao
  0 siblings, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2021-09-06 10:25 UTC (permalink / raw)
  To: Hou Tao; +Cc: Christoph Hellwig, Josef Bacik, Jens Axboe, linux-block, nbd

On Mon, Sep 06, 2021 at 06:08:54PM +0800, Hou Tao wrote:
> >> +	if (!try_module_get(THIS_MODULE))
> >> +		return ERR_PTR(-ENODEV);
> > try_module_get(THIS_MODULE) is an indicator for an unsafe pattern.  If
> > we don't already have a reference it could never close the race.
> >
> > Looking at the callers:
> >
> >  - nbd_open like all block device operations must have a reference
> >    already.
> Yes. nbd_open() has already taken a reference in dentry_open().
> >  - for nbd_genl_connect I'm not an expert, but given that struct
> >    nbd_genl_family has a module member I suspect the networkinh
> >    code already takes a reference.
> 
> That was my original though, but the fact is netlink code doesn't take a module reference
> 
> in genl_family_rcv_msg_doit() and netlink uses genl_lock_all() to serialize between module removal
> 
> and nbd_connect_genl_ops calling, so I think use try_module_get() is OK here.

How it this going to work?  If there was a race you just shortened it,
but it can still happen before you call try_module_get.  So I think we
need to look into how the netlink calling conventions are supposed to
look and understand the issues there first.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-06 10:25       ` Christoph Hellwig
@ 2021-09-07  3:04         ` Hou Tao
  2021-09-08 13:03           ` Hou Tao
  2021-09-09  6:40           ` Christoph Hellwig
  0 siblings, 2 replies; 15+ messages in thread
From: Hou Tao @ 2021-09-07  3:04 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Josef Bacik, Jens Axboe, linux-block, nbd

Hi,

On 9/6/2021 6:25 PM, Christoph Hellwig wrote:
> On Mon, Sep 06, 2021 at 06:08:54PM +0800, Hou Tao wrote:
>>>> +	if (!try_module_get(THIS_MODULE))
>>>> +		return ERR_PTR(-ENODEV);
>>> try_module_get(THIS_MODULE) is an indicator for an unsafe pattern.  If
>>> we don't already have a reference it could never close the race.
>>>
>>> Looking at the callers:
>>>
>>>  - nbd_open like all block device operations must have a reference
>>>    already.
>> Yes. nbd_open() has already taken a reference in dentry_open().
>>>  - for nbd_genl_connect I'm not an expert, but given that struct
>>>    nbd_genl_family has a module member I suspect the networkinh
>>>    code already takes a reference.
>> That was my original though, but the fact is netlink code doesn't take a module reference
>>
>> in genl_family_rcv_msg_doit() and netlink uses genl_lock_all() to serialize between module removal
>>
>> and nbd_connect_genl_ops calling, so I think use try_module_get() is OK here.
> How it this going to work?  If there was a race you just shortened it,
> but it can still happen before you call try_module_get.  So I think we
> need to look into how the netlink calling conventions are supposed to
> look and understand the issues there first.
> .

Let me explain first. The reason it works is due to genl_lock_all() in netlink code.

If the module removal happens before calling try_module_get(), nbd_cleanup() will

call genl_unregister_family() first, and then genl_lock_all(). genl_lock_all() will

prevent ops in nbd_connect_genl_ops() from being called, because the calling

of nbd ops happens in genl_rcv() which needs to acquire cb_lock first.


process A                                       process B

module removal

genl_unregister_family()

  genl_lock_all()

    down_write(&cb_lock)

                                                receive a new netlink message

                                                genl_rcv()

                                                   // will wait for the removal of nbd ops

                                                   down_read(&cb_lock)

If nbd_alloc_config() happens before the module removal, genl_rcv() must

have been acquired cb_lock & genl_mutex, so nbd_cleanup() will block in

genl_unregister_family(). When nbd_alloc_config() calls try_module_get(),

it will find out the module is dying, so fail nbd_genl_connect().


process A                                     process B

a new netlink message

genl_rcv()

  down_read(&cb_lock)

    mutex_lock(&genl_mutex)

      nbd_genl_connect()

        nbd_alloc_config()

                                               module removal

                                               genl_unregister_family

          // module is dying, so fail

          try_module_get()

                                                 genl_lock_all()

                                                   // wait for the completion of nbd ops

                                                   down_write(&cb_lock)

I have checked multiple genl_ops, it seems that the reason why these genl_ops

don't need try_module_get() is that these ops don't create new object through

genl_ops and just control it. However genl_family_rcv_msg_dumpit() will try to

call try_module_get(), but according to the history (6dc878a8ca39 "netlink: add reference of module in netlink_dump_start"),

it is because inet_diag_handler_cmd() will call __netlink_dump_start().

Regards,

Tao



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-07  3:04         ` Hou Tao
@ 2021-09-08 13:03           ` Hou Tao
  2021-09-09  6:40           ` Christoph Hellwig
  1 sibling, 0 replies; 15+ messages in thread
From: Hou Tao @ 2021-09-08 13:03 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Josef Bacik, Jens Axboe, linux-block, nbd

Hi Christoph,

Any comments for this patch ?

On 9/7/2021 11:04 AM, Hou Tao wrote:
> Hi,
>
> On 9/6/2021 6:25 PM, Christoph Hellwig wrote:
>> On Mon, Sep 06, 2021 at 06:08:54PM +0800, Hou Tao wrote:
>>>>> +	if (!try_module_get(THIS_MODULE))
>>>>> +		return ERR_PTR(-ENODEV);
>>>> try_module_get(THIS_MODULE) is an indicator for an unsafe pattern.  If
>>>> we don't already have a reference it could never close the race.
>>>>
>>>> Looking at the callers:
>>>>
>>>>  - nbd_open like all block device operations must have a reference
>>>>    already.
>>> Yes. nbd_open() has already taken a reference in dentry_open().
>>>>  - for nbd_genl_connect I'm not an expert, but given that struct
>>>>    nbd_genl_family has a module member I suspect the networkinh
>>>>    code already takes a reference.
>>> That was my original though, but the fact is netlink code doesn't take a module reference
>>>
>>> in genl_family_rcv_msg_doit() and netlink uses genl_lock_all() to serialize between module removal
>>>
>>> and nbd_connect_genl_ops calling, so I think use try_module_get() is OK here.
>> How it this going to work?  If there was a race you just shortened it,
>> but it can still happen before you call try_module_get.  So I think we
>> need to look into how the netlink calling conventions are supposed to
>> look and understand the issues there first.
>> .
> Let me explain first. The reason it works is due to genl_lock_all() in netlink code.
>
> If the module removal happens before calling try_module_get(), nbd_cleanup() will
>
> call genl_unregister_family() first, and then genl_lock_all(). genl_lock_all() will
>
> prevent ops in nbd_connect_genl_ops() from being called, because the calling
>
> of nbd ops happens in genl_rcv() which needs to acquire cb_lock first.
>
>
> process A                                       process B
>
> module removal
>
> genl_unregister_family()
>
>   genl_lock_all()
>
>     down_write(&cb_lock)
>
>                                                 receive a new netlink message
>
>                                                 genl_rcv()
>
>                                                    // will wait for the removal of nbd ops
>
>                                                    down_read(&cb_lock)
>
> If nbd_alloc_config() happens before the module removal, genl_rcv() must
>
> have been acquired cb_lock & genl_mutex, so nbd_cleanup() will block in
>
> genl_unregister_family(). When nbd_alloc_config() calls try_module_get(),
>
> it will find out the module is dying, so fail nbd_genl_connect().
>
>
> process A                                     process B
>
> a new netlink message
>
> genl_rcv()
>
>   down_read(&cb_lock)
>
>     mutex_lock(&genl_mutex)
>
>       nbd_genl_connect()
>
>         nbd_alloc_config()
>
>                                                module removal
>
>                                                genl_unregister_family
>
>           // module is dying, so fail
>
>           try_module_get()
>
>                                                  genl_lock_all()
>
>                                                    // wait for the completion of nbd ops
>
>                                                    down_write(&cb_lock)
>
> I have checked multiple genl_ops, it seems that the reason why these genl_ops
>
> don't need try_module_get() is that these ops don't create new object through
>
> genl_ops and just control it. However genl_family_rcv_msg_dumpit() will try to
>
> call try_module_get(), but according to the history (6dc878a8ca39 "netlink: add reference of module in netlink_dump_start"),
>
> it is because inet_diag_handler_cmd() will call __netlink_dump_start().
>
> Regards,
>
> Tao
>
>
> .

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-07  3:04         ` Hou Tao
  2021-09-08 13:03           ` Hou Tao
@ 2021-09-09  6:40           ` Christoph Hellwig
  2021-09-13  4:32             ` Hou Tao
  1 sibling, 1 reply; 15+ messages in thread
From: Christoph Hellwig @ 2021-09-09  6:40 UTC (permalink / raw)
  To: Hou Tao; +Cc: Christoph Hellwig, Josef Bacik, Jens Axboe, linux-block, nbd

On Tue, Sep 07, 2021 at 11:04:16AM +0800, Hou Tao wrote:
> Let me explain first. The reason it works is due to genl_lock_all() in netlink code.

Btw, please properly format your mail.  These overly long lines are really
hard to read.

> If the module removal happens before calling try_module_get(), nbd_cleanup() will
> 
> call genl_unregister_family() first, and then genl_lock_all(). genl_lock_all() will
> 
> prevent ops in nbd_connect_genl_ops() from being called, because the calling
> 
> of nbd ops happens in genl_rcv() which needs to acquire cb_lock first.

Good.

> I have checked multiple genl_ops, it seems that the reason why these genl_ops
> 
> don't need try_module_get() is that these ops don't create new object through
> 
> genl_ops and just control it. However genl_family_rcv_msg_dumpit() will try to
> 
> call try_module_get(), but according to the history (6dc878a8ca39 "netlink: add reference of module in netlink_dump_start"),
> 
> it is because inet_diag_handler_cmd() will call __netlink_dump_start().

And now taking a step back:  Why do we even need this extra module
reference?  For the case where nbd_alloc_config is called from nbd_open
we obviously don't need it.  In the case where it is called from
nbd_genl_connect that prevents unloading nbd when there is a configured
but not actually nbd device.  Which isn't reallyed need and counter to
how other drivers work.

Did you try just removing the extra module refcounting?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-09  6:40           ` Christoph Hellwig
@ 2021-09-13  4:32             ` Hou Tao
  2021-09-13 15:25               ` Christoph Hellwig
  2021-09-14 11:42               ` Wouter Verhelst
  0 siblings, 2 replies; 15+ messages in thread
From: Hou Tao @ 2021-09-13  4:32 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Josef Bacik, Jens Axboe, linux-block, nbd

Hi Christoph,

On 9/9/2021 2:40 PM, Christoph Hellwig wrote:
> On Tue, Sep 07, 2021 at 11:04:16AM +0800, Hou Tao wrote:
>> Let me explain first. The reason it works is due to genl_lock_all() in netlink code.
> Btw, please properly format your mail.  These overly long lines are really
> hard to read.
Thanks for reminding.
>> If the module removal happens before calling try_module_get(), nbd_cleanup() will
>>
>> call genl_unregister_family() first, and then genl_lock_all(). genl_lock_all() will
>>
>> prevent ops in nbd_connect_genl_ops() from being called, because the calling
>>
>> of nbd ops happens in genl_rcv() which needs to acquire cb_lock first.
> Good.
>
>> I have checked multiple genl_ops, it seems that the reason why these genl_ops
>>
>> don't need try_module_get() is that these ops don't create new object through
>>
>> genl_ops and just control it. However genl_family_rcv_msg_dumpit() will try to
>>
>> call try_module_get(), but according to the history (6dc878a8ca39 "netlink: add reference of module in netlink_dump_start"),
>>
>> it is because inet_diag_handler_cmd() will call __netlink_dump_start().
> And now taking a step back:  Why do we even need this extra module
> reference?  For the case where nbd_alloc_config is called from nbd_open
> we obviously don't need it.  In the case where it is called from
> nbd_genl_connect that prevents unloading nbd when there is a configured
> but not actually nbd device.  Which isn't reallyed need and counter to
> how other drivers work.
Yes, the purpose of module ref-counting in nbd_alloc_config() is to force
the user to disconnect the nbd device manually before module removal.
And loop device works in the same way. If a file is attached to a loop device,
an extra module reference will be taken in loop_configure() and the removal
of loop module will fail. The only difference is that loop driver takes the
extra ref-count by ioctl, and nbd does it through netlink.
>
> Did you try just removing the extra module refcounting?
Yes, removing the module refcounting in nbd_alloc_config() and cleaning
the nbd_config in nbd_cleanup() also work, but not sure whether or not
it will break some nbd user-case which depends on the extra module
reference count. I prefer to keep the extra module refcounting considering
that loop driver does it as well, so what is your suggestion ?

Regards,
Tao

> .


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-13  4:32             ` Hou Tao
@ 2021-09-13 15:25               ` Christoph Hellwig
  2021-09-14 11:42               ` Wouter Verhelst
  1 sibling, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2021-09-13 15:25 UTC (permalink / raw)
  To: Hou Tao; +Cc: Christoph Hellwig, Josef Bacik, Jens Axboe, linux-block, nbd

On Mon, Sep 13, 2021 at 12:32:37PM +0800, Hou Tao wrote:
> > Did you try just removing the extra module refcounting?
> Yes, removing the module refcounting in nbd_alloc_config() and cleaning
> the nbd_config in nbd_cleanup() also work, but not sure whether or not
> it will break some nbd user-case which depends on the extra module
> reference count. I prefer to keep the extra module refcounting considering
> that loop driver does it as well, so what is your suggestion ?

Can you respin the patch with a comment explaining this in detail
so that the next person tripping over it doesn't have to do the research
again?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal
  2021-09-13  4:32             ` Hou Tao
  2021-09-13 15:25               ` Christoph Hellwig
@ 2021-09-14 11:42               ` Wouter Verhelst
  1 sibling, 0 replies; 15+ messages in thread
From: Wouter Verhelst @ 2021-09-14 11:42 UTC (permalink / raw)
  To: Hou Tao; +Cc: Christoph Hellwig, Josef Bacik, Jens Axboe, linux-block, nbd

On Mon, Sep 13, 2021 at 12:32:37PM +0800, Hou Tao wrote:
> Hi Christoph,
> 
> On 9/9/2021 2:40 PM, Christoph Hellwig wrote:
> > On Tue, Sep 07, 2021 at 11:04:16AM +0800, Hou Tao wrote:
> >> Let me explain first. The reason it works is due to genl_lock_all() in netlink code.
> > Btw, please properly format your mail.  These overly long lines are really
> > hard to read.
> Thanks for reminding.
> >> If the module removal happens before calling try_module_get(), nbd_cleanup() will
> >>
> >> call genl_unregister_family() first, and then genl_lock_all(). genl_lock_all() will
> >>
> >> prevent ops in nbd_connect_genl_ops() from being called, because the calling
> >>
> >> of nbd ops happens in genl_rcv() which needs to acquire cb_lock first.
> > Good.
> >
> >> I have checked multiple genl_ops, it seems that the reason why these genl_ops
> >>
> >> don't need try_module_get() is that these ops don't create new object through
> >>
> >> genl_ops and just control it. However genl_family_rcv_msg_dumpit() will try to
> >>
> >> call try_module_get(), but according to the history (6dc878a8ca39 "netlink: add reference of module in netlink_dump_start"),
> >>
> >> it is because inet_diag_handler_cmd() will call __netlink_dump_start().
> > And now taking a step back:  Why do we even need this extra module
> > reference?  For the case where nbd_alloc_config is called from nbd_open
> > we obviously don't need it.  In the case where it is called from
> > nbd_genl_connect that prevents unloading nbd when there is a configured
> > but not actually nbd device.  Which isn't reallyed need and counter to
> > how other drivers work.
> Yes, the purpose of module ref-counting in nbd_alloc_config() is to force
> the user to disconnect the nbd device manually before module removal.
> And loop device works in the same way. If a file is attached to a loop device,
> an extra module reference will be taken in loop_configure() and the removal
> of loop module will fail. The only difference is that loop driver takes the
> extra ref-count by ioctl, and nbd does it through netlink.

Haven't checked the actual patch, but just wanted to point out:

nbd should do it through netlink *and* ioctl -- the older way to
configure nbd was through ioctl, which we should still support for
backcompat reasons.

(if that's already the case, then ignore what I said :-)

-- 
     w@uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-09-14 11:42 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-04 12:25 [PATCH v2 0/3] fix races between nbd setup and module removal Hou Tao
2021-09-04 12:25 ` [PATCH v2 1/3] nbd: use pr_err to output error message Hou Tao
2021-09-06  9:27   ` Christoph Hellwig
2021-09-04 12:25 ` [PATCH v2 2/3] nbd: call genl_unregister_family() first in nbd_cleanup() Hou Tao
2021-09-06  9:27   ` Christoph Hellwig
2021-09-04 12:25 ` [PATCH v2 3/3] nbd: fix race between nbd_alloc_config() and module removal Hou Tao
2021-09-06  9:30   ` Christoph Hellwig
2021-09-06 10:08     ` Hou Tao
2021-09-06 10:25       ` Christoph Hellwig
2021-09-07  3:04         ` Hou Tao
2021-09-08 13:03           ` Hou Tao
2021-09-09  6:40           ` Christoph Hellwig
2021-09-13  4:32             ` Hou Tao
2021-09-13 15:25               ` Christoph Hellwig
2021-09-14 11:42               ` Wouter Verhelst

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).