From patchwork Wed Feb 28 17:38:34 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Boaz Harrosh X-Patchwork-Id: 10249011 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A085360211 for ; Wed, 28 Feb 2018 17:38:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8D6D628DB1 for ; Wed, 28 Feb 2018 17:38:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8214D28DC0; Wed, 28 Feb 2018 17:38:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6D88828DB6 for ; Wed, 28 Feb 2018 17:38:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932537AbeB1Rij (ORCPT ); Wed, 28 Feb 2018 12:38:39 -0500 Received: from mail-wm0-f53.google.com ([74.125.82.53]:52066 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934005AbeB1Rii (ORCPT ); Wed, 28 Feb 2018 12:38:38 -0500 Received: by mail-wm0-f53.google.com with SMTP id h21so6595054wmd.1 for ; Wed, 28 Feb 2018 09:38:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=y0uMqysUd5kXqTvSo5HB0oNSYGZ0ZtQGa80EWXSXLgY=; b=gDACQl84JHmCiMvYh9l5o44ziJRLx+hfMIZm0/5ystUW/rEVEFrSR5kUa8x9f7Okge D+IXUpvDnmLmoERwDvLAallQgjElGuUvnSt45sIpXwJh2avKtdYTKFc4MLg5pOjKldvn JJE/pbW/PqQlKrFJ3kCctiil2EQD8d9q92BLalRMi+2lK1PtFfAL1VITzn1abR47pfHe x3AtYyrqDa38CSOsi6PqeaQ+GtkKZU+sNIt7WC91MdKQfM76YH/EQIOfu2M0i3F9SVyv o27NqEQRPs3g8GlEEFDMql08Tr8U+RpERoB4G5EqX+e65IXS03xzHRGtd7XoFx8EakzU FxAA== X-Gm-Message-State: APf1xPBlDsN27zMPEXjSWwZO8XV7Sikbw3FcYymuZaNB0xWp0W4/szXq BWNpFG7PxuPPUt3MPO3kpP4= X-Google-Smtp-Source: AG47ELtKkgFm23WM9uydxq37Jv8fCefp2GSG8fnpJKBZhV5a+RmSsQCVdU7LaPZRCEn7+bwCLpWLNw== X-Received: by 10.28.74.66 with SMTP id x63mr13595276wma.4.1519839516638; Wed, 28 Feb 2018 09:38:36 -0800 (PST) Received: from [10.0.0.5] ([207.232.55.62]) by smtp.gmail.com with ESMTPSA id p5sm2670415wmf.13.2018.02.28.09.38.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 28 Feb 2018 09:38:35 -0800 (PST) Subject: Re: [PATCH] Use bio_endio instead of bio_put in error path of blk_rq_append_bio To: Jiri Palecek , Ming Lei References: <20171218074044.1369-3-ming.lei@redhat.com> <87o9lcp1aq.fsf@debian.i-did-not-set--mail-host-address--so-tickle-me> <2c6a3916-57b8-20cb-f081-bb1b907a1a8a@web.de> <20180131052448.GB9985@ming.t460p> <87h8qbptzz.fsf@debian.i-did-not-set--mail-host-address--so-tickle-me> <8be6c1f5-0359-8de4-22b3-d5e779ae7c12@electrozaur.com> <058e8999-3f89-ae20-ee38-229c78d6d1c8@web.de> Cc: Ming Lei , linux-block , Jens Axboe , Christoph Hellwig , "James E.J. Bottomley" , "Martin K. Petersen" , "Nicholas A. Bellinger" From: Boaz Harrosh Message-ID: <24729d65-c531-dcc5-3eca-900d65961092@electrozaur.com> Date: Wed, 28 Feb 2018 19:38:34 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <058e8999-3f89-ae20-ee38-229c78d6d1c8@web.de> Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 27/02/18 16:44, Jiri Palecek wrote: <> >> These are BIDI commands that travel as a couple of chained requests. They are >> sent as BLOCK_PC command and complete or fail as one hole unit. The system is not >> allowed (And does not know how) to split them or complete them partially. This is >> not an FS read or write request of data. But a unique OSD request that the system >> knows nothing about. The all scsi-command is specially crafted by the caller. >> It is all properly destroyed at osd_request end of life (sync or async) >> (NOTE there is no bio-end function only req-end) > > That's correct, however, the assumed leak would happen when the > preparation of the request fails. > > The whole issue started with possible bounce buffer leaks in > blk_rq_append_request, which didn't happen before it started to > handle bouncing. However, Ming Lei noticed it would be simpler and > easier to also free the original bio, as opposed to just the bounce > buffer, on error. Because that wasn't so previously, the question now > stands, what do the callers likely expect? > Sigh! That bouncing thing. It is so not relevant for osd. I wish we could just drop the bouncing and return an error, if it was ever needed (because it never is for osd) > This leads us to osd_initiator.c, which IMHO doesn't expect anything > and simply lacks any sane handling of such possibility. If you say > there can't be any leaks, please explain to me this riddle. Given the > code: > > static struct request *_make_request(struct request_queue *q, bool has_write, > struct _osd_io_info *oii, gfp_t flags) > { > struct request *req; > struct bio *bio = oii->bio; > int ret; > > req = blk_get_request(q, has_write ? REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, > flags); /** 1 **/ > if (IS_ERR(req)) > return req; > > for_each_bio(bio) { > struct bio *bounce_bio = bio; > > ret = blk_rq_append_bio(req, &bounce_bio); [Sorry still did not see the all code] What is the meaning of the pointer-to-pointer-to-bio here? it used to be relevant to blk_queue_bounce, that I guess you internalized into blk_rq_append_bio() but why do we need it outside the call now? > if (ret) > return ERR_PTR(ret); /** 2 **/ > } > > return req; > } > > 1. If this function exits by line labeled "2", where is struct > request *req allocated on line 1 freed? You are probably right, if you want to fix it? This used to never fail because blk_queue_bounce() retuning void. And blk_rq_append_bio could only fail if ll_back_merge_fn would fail, but for OSD supporting drivers this would never fail. And also those checks were already preformed before (when more segments were added). So in practice this could never fail and never showed in testing. But I agree this is a layering violation and the code is wrong. > 2. If this function exits by line 2, where are the bio(s) in oii->bio > freed? What if it fails on the second bio? > > I assume osd_end_request would be called afterwards (as in exofs_read_kern). > This code always scared me to bits because if you look closely it does nothing It does not actually builds the chain it relays on bio->bi_next not being modified and so we loop on doing nothing (resting the bio->bi_next to the same thing it was before) So yes oii->bio(s) are freed in osd_end_request and/or by callers because the bios might have one more ref on them, because they might be needed by caller to finish up. > Regards > Jiri Palecek > Thank you for looking into this, sorry for the headache this code gives you. If there was a way to just fail if bounce is needed that could be just fine for osd. But else I guess that error exit needs fixing. Based on v4.15 code feel free to add to your patchset where needed ---- Don't leak the request if blk_rq_append_bio fails Sign-of-by: Boaz Harrosh ---- diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c index 2f2a991..3c97e0e 100644 --- a/drivers/scsi/osd/osd_initiator.c +++ b/drivers/scsi/osd/osd_initiator.c @@ -1572,8 +1572,10 @@ static struct request *_make_request(struct request_queue *q, bool has_write, blk_queue_bounce(req->q, &bounce_bio); ret = blk_rq_append_bio(req, bounce_bio); - if (ret) + if (ret) { + _put_request(req); return ERR_PTR(ret); + } } return req;