| Message ID | 87o9lcp1aq.fsf@debian.i-did-not-set--mail-host-address--so-tickle-me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Thanks, Ming Lei
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. 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. Regards Jiri Palecek
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? > > 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. Thanks, Ming
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@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? 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
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 = orig_bio; } return -EINVAL;