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