| Message ID | 87h8qbptzz.fsf@debian.i-did-not-set--mail-host-address--so-tickle-me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/02/18 17:21, Jiri Palecek wrote: > Hello, > > I had a look at the callers of blk_rq_append_bio and checked the > callers. Some changes may need to be done there and I'd like the input > of their maintainers as well before finalising the patch. > > Ming Lei <ming.lei@redhat.com> writes: > >> On Tue, Jan 30, 2018 at 04:24:14PM +0100, Jiri Palecek wrote: >>> >>> On 1/30/18 1:53 PM, Ming Lei wrote: >>>> On Thu, Jan 25, 2018 at 9:58 PM, Jiří Paleček <jpalecek@web.de> wrote: >>>>> Avoids page leak from bounced requests >>>>> --- >>>>> block/blk-map.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/block/blk-map.c b/block/blk-map.c >>>>> index d3a94719f03f..702d68166689 100644 >>>>> --- a/block/blk-map.c >>>>> +++ b/block/blk-map.c >>>>> @@ -26,7 +26,8 @@ int blk_rq_append_bio(struct request *rq, struct bio **bio) >>>>> } else { >>>>> if (!ll_back_merge_fn(rq->q, rq, *bio)) { >>>>> if (orig_bio != *bio) { >>>>> - bio_put(*bio); >>>>> + bio_inc_remaining(orig_bio); >>>>> + bio_endio(*bio); >>>> 'bio_inc_remaining(orig_bio);' shouldn't be needed since we don't chain bounced >>>> bio, otherwise this patch is fine. >>> >>> I believe it is needed or at least desirable. The situation when the request >>> bounced is like this >>> >>> bio (bounced) . bi_private ---> orig_bio >>> >>> and at the end of bounce_end_io, bio_endio(bio->bi_private) [which is >>> bio_endio(orig_bio) in our case] is called. That doesn't have any effect on >>> __blk_rq_map_user_iov; its bios have .bi_end_io==0, therefore one call more >>> or less doesn't matter. However, for other callers, like osd_initiator.c, >>> this is not the case. They pass bios which have bi_end_io, and might be >>> surprised if this was called unexpectedly. >> >> OK, I think it is good to follow the rule of not calling .bi_end_io() in >> the failure path, just like __blk_rq_map_user_iov()/blk_rq_map_kern(). >> >> But seems pscsi_map_sg() depends on bio_endio(orig_bio) to free the 'orig_bio', >> could you fix it in this patch too? > > I've come up with the following patch. Some notes: > > 1) First of all, I think your suggestion to call bio_endio of the > original bio in blk_rq_append_bio is right. It would make things a > bit easier for the callers and those who I suspected need to hold on > the bio are actually just ignorant about errors. However, it does > change the api. So if you agree and the other parts are OK too, I'd > make a patch without the bio_inc_remaining. > > 2) The osd_initiator.c seems to be a bit oblivious about errors, leaking > not only the bios, but the request as well. I added some functions > for proper cleanup there, with considerations: > > - I think it's better to unhook the .bi_next link of the bio before > adding it to the request, because blk_rq_append_bio uses that for > its own purposes. > > - Once the bios from osd_request have been spent (added to a > blk_request), they can be NULL-ified. > I have not followed closely the issue but ... No! the osd_initiator.c is completely out of scope. And does not leak any bios and need not to be fixed. let me explain. 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) again not followed closely but if it is about the free of the bounce buffers then I would just disable bouncing for osd all together. There are only a very few (2) drivers that support bidi for osd. iscsi and iser so we know those are totally cool and this is all not an existing problem. Thanks Boaz > I'd like hear if these are OK. > > 3) PSCSI needs to free the bios in pscsi_map_sg, but also needs to clear > the bios from the request in pscsi_execute_cmd in case of partial > errors (ie. first sg segment makes it to the request, second > fails). To my understanding, blk_put_request is insufficient in that > case. > > Regards > Jiri Palecek >
On 21/02/18 18:20, Boaz Harrosh wrote: <> > again not followed closely but if it is about the free of the bounce buffers > then I would just disable bouncing for osd all together. There are only a very > few (2) drivers that support bidi for osd. iscsi and iser so we know those are > totally cool and this is all not an existing problem. > And also it is possible that I have at all not understood the issue please bring me up to speed. I would like to help any way I can. > Thanks > Boaz >
From fca495c6bf41775be152e7f7f00be6f0dc746ac3 Mon Sep 17 00:00:00 2001 From: =?iso8859-2?q?Ji=F8=ED=20Pale=E8ek?= <jpalecek@web.de> Date: Sun, 4 Feb 2018 21:55:56 +0100 Subject: [PATCH 2/2] Some error paths --- block/blk-map.c | 4 +++- drivers/scsi/osd/osd_initiator.c | 29 +++++++++++++++++++++++++---- drivers/target/target_core_pscsi.c | 4 +++- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/block/blk-map.c b/block/blk-map.c index 702d68166689..8378f4ddd419 100644 --- a/block/blk-map.c +++ b/block/blk-map.c @@ -246,7 +246,7 @@ int blk_rq_map_kern(struct request_queue *q, struct request *rq, void *kbuf, ret = blk_rq_append_bio(rq, &bio); if (unlikely(ret)) { /* request is too big */ - bio_put(orig_bio); + bio_endio(orig_bio); return ret; } diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c index e18877177f1b..f352fdda52b9 100644 --- a/drivers/scsi/osd/osd_initiator.c +++ b/drivers/scsi/osd/osd_initiator.c @@ -437,6 +437,16 @@ static void _osd_free_seg(struct osd_request *or __unused, seg->alloc_size = 0; } +static void _osd_end_bios(struct bio *bio) +{ + struct bio *nxt = NULL; + while (bio) { + nxt = bio->bi_next; + bio_endio(bio); + bio = nxt; + } +} + static void _put_request(struct request *rq) { /* @@ -469,6 +479,10 @@ void osd_end_request(struct osd_request *or) _osd_free_seg(or, &or->set_attr); _osd_free_seg(or, &or->cdb_cont); + /* Only in case of errors should these be non-NULL */ + _osd_end_bios(or->in.bio); + _osd_end_bios(or->out.bio); + _osd_request_free(or); } EXPORT_SYMBOL(osd_end_request); @@ -1575,13 +1589,20 @@ static struct request *_make_request(struct request_queue *q, bool has_write, if (IS_ERR(req)) return req; - for_each_bio(bio) { - struct bio *bounce_bio = bio; + while(bio) { + struct bio *nxt = bio->bi_next; + bio->bi_next = NULL; - ret = blk_rq_append_bio(req, &bounce_bio); - if (ret) + ret = blk_rq_append_bio(req, &bio); + if (ret) { + _put_request(req); + bio_endio(bio); + oii->bio = nxt; return ERR_PTR(ret); + } + bio = nxt; } + oii->bio = NULL; return req; } diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 0d99b242e82e..42c24356d683 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -922,6 +922,7 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, rc = blk_rq_append_bio(req, &bio); if (rc) { + bio_put(bio); pr_err("pSCSI: failed to append bio\n"); goto fail; } @@ -940,6 +941,7 @@ pscsi_map_sg(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents, if (bio) { rc = blk_rq_append_bio(req, &bio); if (rc) { + bio_put(bio); pr_err("pSCSI: failed to append bio\n"); goto fail; } @@ -1016,7 +1018,7 @@ pscsi_execute_cmd(struct se_cmd *cmd) return 0; fail_put_request: - blk_put_request(req); + blk_end_request_all(req, BLK_STS_IOERR); fail: kfree(pt); return ret; -- 2.15.1