linux-kernel - Re: [jet.chen@...el.com: [bio] kernel BUG at drivers/block/virtio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 May 2014 10:44:00 +0200
From:	Maurizio Lombardi <mlombard@...hat.com>
To:	Ming Lei <tom.leiming@...il.com>
Cc:	Jens Axboe <axboe@...nel.dk>, jet.chen@...el.com,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	LKML <linux-kernel@...r.kernel.org>, lkp@...org,
	Fengguang Wu <fengguang.wu@...el.com>
Subject: Re: [jet.chen@...el.com: [bio] kernel BUG at
 drivers/block/virtio_blk.c:166!]

On Tue, May 27, 2014 at 12:03:29PM +0800, Ming Lei wrote:
> On Tue, May 27, 2014 at 3:43 AM, Maurizio Lombardi <mlombard@...hat.com> wrote:
> > Hi Jens,
> >
> > looks like that commit 3979ef4dcf3d1de55a560a3a4016c30a835df44d ("bio-modify-__bio_add_page-to-accept-pages-that-dont-start-a-new-segment-v3")
> > introduces a regression, as reported by Jet Chan.
> >
> > Do you have any idea about the possible problem with this patch?
> >
> > it is the one that performs a recount of the segments in case of failure in __bio_add_page()
> >
> > http://www.spinics.net/lists/mm-commits/msg103684.html
> >
> > I would not be surprised if the bug was introduced by fceb38f36f, because it
> > contained a mystake that commit 3979ef4dcf supposedly fixed.
> > But learning that commit 3979ef4dcf is introducing a regression leaves
> > me quite puzzled.
> 
> From code of __blk_recalc_rq_segments(), looks it
> won't check if recounted physical segment number is
> bigger than queue_max_segments(), so wondering if
> blk_recount_segments() can always decrease
> physical segment number.
>

This is what __bio_add_page() did before both fceb38f36f and 3979ef4dcf at line 757


        while (bio->bi_phys_segments >= queue_max_segments(q)) {

                if (retried_segments)
                        return 0;

                retried_segments = 1;
                blk_recount_segments(q, bio);
        }

so it is possible, in case of error, to return from the function even if the recounted
physical segments are bigger than queue_max_segments(q).

---------

But now I'm suspicious of this part of commit 3979ef4dcf:

 failed:
        bvec->bv_page = NULL;
        bvec->bv_len = 0;
        bvec->bv_offset = 0;
        bio->bi_vcnt--;  <----------------
        blk_recount_segments(q, bio);
        return 0;

Is decreasing bi_vcnt sufficient to guarantee that blk_recount_segments()
recalculates the correct number of physical segments?
Looking at the __blk_recalc_rq_segments() it appears it may not be the case.

The question is how can we restore the correct number of physical segments in case
of failure without breaking anything...

Regards,
Maurizio Lombardi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/