Messages in this thread | | | Date | Tue, 7 Feb 2023 08:54:30 +0800 | Subject | Re: [PATCH] rbd: avoid double free memory on error path in rbd_dev_create() | From | Xiubo Li <> |
| |
On 06/02/2023 23:15, Петрова Наталия Михайловна wrote: > Hi Ilya! > Thanks for your response! I don't quite understand your idea and suggestion. The patch is designed to avoid double free memory. I explored the code again and suppose there is another situation for rbd_dev->rbd_client and rbd_dev->spec. Free memory of these pointers is possible only once in rbd_dev_free() function. In do_rbd_add() deallocation memory is only for rbd_opts: drivers/block/rbd.c 7157.
Hi Петрова,
If the rbd_dev_create() fails, for spec it will be freed in rbd_dev_create()->rbd_spec_put() first and then in do_rbd_add() it will call rbd_spec_put() again.
It won't trigger double free but this should generate a warning when the refcount underflow, because the refcount_dec_and_test() will warn and then return false when underflow happens.
The same for rbd_client.
Thanks,
- Xiubo
> Correct me if I'm wrong. > > Thanks, > Natalia > > -----Original Message----- > From: Ilya Dryomov <idryomov@gmail.com> > Sent: Monday, February 6, 2023 2:59 PM > To: Петрова Наталия Михайловна <n.petrova@fintech.ru> > Cc: Dongsheng Yang <dongsheng.yang@easystack.cn>; Jens Axboe <axboe@kernel.dk>; ceph-devel@vger.kernel.org; linux-block@vger.kernel.org; linux-kernel@vger.kernel.org; lvc-project@linuxtesting.org; Alexey Khoroshilov <khoroshilov@ispras.ru> > Subject: Re: [PATCH] rbd: avoid double free memory on error path in rbd_dev_create() > > On Fri, Feb 3, 2023 at 3:15 PM Natalia Petrova <n.petrova@fintech.ru> wrote: >> If rbd_dev_create() fails after assignment 'opts' to 'rbd_dev->opts', >> double free of 'rbd_options' happens: >> one is in rbd_dev_free() and another one is in do_rbd_add(). >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Fixes: 1643dfa4c2c8 ("rbd: introduce a per-device ordered workqueue") >> Signed-off-by: Natalia Petrova <n.petrova@fintech.ru> >> Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> >> --- >> drivers/block/rbd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index >> 04453f4a319c..ab6bfc352cde 100644 >> --- a/drivers/block/rbd.c >> +++ b/drivers/block/rbd.c >> @@ -5357,7 +5357,6 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc, >> if (!rbd_dev) >> return NULL; >> >> - rbd_dev->opts = opts; >> >> /* get an id and fill in device name */ >> rbd_dev->dev_id = ida_simple_get(&rbd_dev_id_ida, 0, @@ >> -5372,6 +5371,7 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc, >> if (!rbd_dev->task_wq) >> goto fail_dev_id; >> >> + rbd_dev->opts = opts; >> /* we have a ref from do_rbd_add() */ >> __module_get(THIS_MODULE); >> >> -- >> 2.34.1 >> > Hi Natalia, > > It seems like a similar issue is affecting rbd_dev->rbd_client and rbd_dev->spec. Unlike rbd_dev->opts, they are ref-counted and I'm guessing that the verification tool doesn't go that deep. > > I'd prefer all three to be addressed in the same change, since it's the same error path. Would you be willing to look into that and post a new revision or should I treat just this patch as a bug report? > > Thanks, > > Ilya
-- Best Regards,
Xiubo Li (李秀波)
Email: xiubli@redhat.com/xiubli@ibm.com Slack: @Xiubo Li
| |