* [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).