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?
>
> 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
