On 1/31/18 6:24 AM, Ming Lei wrote:
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@xxxxxx> 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?
Yes, there seems to be leak in the error path of that code. However,
that was there for more than a year so I didn't think that was urgent.
I'll have a look at it, but I would prefer if someone familiar with
pscsi had their say as well.
Before caa4b02476e3, blk_rq_append_request wouldn't touch/delete the passed
bio at all under any circumstances. I believe it should stay that way and
incrementing the remaining counter, which effectively nullifies the extra
bio_endio call, does that pretty straightforwardly.
Seems too tricky to use bio_inc_remaining() for avoiding bio_endio(orig_bio),
if we have to do that, I suggest to add comment on that.
Okay. I thought that it didn't really reach the level of sophistication
otherwise seen in block layer code :)
Regards
Jiri Palecek
