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@xxxxxxxxxx>
> ---
> 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.
