Message ID | 20230203141515.125205-1-n.petrova@fintech.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp867855wrn; Fri, 3 Feb 2023 06:26:35 -0800 (PST) X-Google-Smtp-Source: AK7set9hp1zsPHLD2uoMIbEL6tjvRXamDN9/puJ0LTw1EKc0eneUNjoQgJrBYHyKhUXxSb7Rxjev X-Received: by 2002:a17:906:a1c6:b0:878:816f:8691 with SMTP id bx6-20020a170906a1c600b00878816f8691mr10226138ejb.71.1675434395460; Fri, 03 Feb 2023 06:26:35 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1675434395; cv=none; d=google.com; s=arc-20160816; b=Kra8ATO8E8keITO7/U1sOnPiPl9B0y/oAnKL9o5x4GYWQYdOCmU4G44efcDULePsbe AsW0ricGt96UOVkhfB3YNbw+M1Dvy37h0U4gYH+PnG7+2XOjEDmH79EWmU2VmOEBiFrQ e1C0NvMaUREhFbekEGVtakdAG3z4/oJjx8n4e7QYEiQe9vMmv7RjJcXI6iFv3EkE9lGJ 7gz7AoSIwl/8S1BR39JviAYXQsdDkc7zuh08eGHwngrJmAZFZhEx+XE7CJizF1sSFz10 Lsdbn2oHa9/2f3BUYqNB0H2UyNkkKQXO1YyrrmVlGycYumOStlg43BueNH7xs94WGgIh FHKQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=x/egGhVQ8N3gzCs0CDTWWueHl4WTsjgdj/JRyYGmVRU=; b=Cc83r5sG1gU9oGG7aeTjJpcsFyywVrEvE06Pr+I1UlFeToMuWluEJ6eglXgc18OVie XB3i51csw0nsDduJYzTbs7VmoJiGIiMkVWwJUw/O0Y0aPg9WbGQ6+3CJ/l8vBUCw7+R3 2tW1k+DLFSw9678OLO6pqU6up/MBCF2QpDzSAZprTR4VtI83QtFUdYFVq8HdMfQ75ih4 tdNd/CqAbXM/ZSHkv2nflLPK/dbcBYb6AyHDC6wdpJ63TwJQqVXFVhhlcmPIavFZLYWH KBrRv7iaCceTb8WhpLSZvvq49ldzfD6/Y6G6DTyVzZ9TMXDctx7xefrVcfojJ2Cx2Xay suXg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id e6-20020a50ec86000000b0048793b0993asi2631242edr.390.2023.02.03.06.26.11; Fri, 03 Feb 2023 06:26:35 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233792AbjBCOQf (ORCPT <rfc822;il.mystafa@gmail.com> + 99 others); Fri, 3 Feb 2023 09:16:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59818 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232942AbjBCOQD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 3 Feb 2023 09:16:03 -0500 Received: from exchange.fintech.ru (exchange.fintech.ru [195.54.195.159]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AC2891F5F5; Fri, 3 Feb 2023 06:15:19 -0800 (PST) Received: from Ex16-01.fintech.ru (10.0.10.18) by exchange.fintech.ru (195.54.195.169) with Microsoft SMTP Server (TLS) id 14.3.498.0; Fri, 3 Feb 2023 17:15:17 +0300 Received: from KANASHIN1.fintech.ru (10.0.253.125) by Ex16-01.fintech.ru (10.0.10.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Fri, 3 Feb 2023 17:15:17 +0300 From: Natalia Petrova <n.petrova@fintech.ru> To: Ilya Dryomov <idryomov@gmail.com> CC: Natalia Petrova <n.petrova@fintech.ru>, 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: [PATCH] rbd: avoid double free memory on error path in rbd_dev_create() Date: Fri, 3 Feb 2023 17:15:15 +0300 Message-ID: <20230203141515.125205-1-n.petrova@fintech.ru> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [10.0.253.125] X-ClientProxiedBy: Ex16-01.fintech.ru (10.0.10.18) To Ex16-01.fintech.ru (10.0.10.18) X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1756820296656648320?= X-GMAIL-MSGID: =?utf-8?q?1756820296656648320?= |
Series |
rbd: avoid double free memory on error path in rbd_dev_create()
|
|
Commit Message
Natalia Petrova
Feb. 3, 2023, 2:15 p.m. UTC
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(-)
Comments
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
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. 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
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
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);