Re: [PATCH] rbd: avoid double free memory on error path in rbd_dev_create()

[Xiubo Li <xiubli@redhat.com>]

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