| Message ID | 20210904122519.1963983-4-houtao1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix races between nbd setup and module removal | expand |
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.
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. > .
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.
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
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 > > > .
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?
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 > .
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?
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 :-)
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);
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(-)