linux-kernel - Re: e827091cb1 "block: merge: get the 1st and last bvec via helpers" broken

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:	Sat, 12 Mar 2016 22:25:48 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Kent Overstreet <kent.overstreet@...il.com>
Cc:	Ming Lin <mlin@...nel.org>, Jens Axboe <axboe@...nel.dk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	torvalds@...ux-foundation.org, tom.leiming@...il.com
Subject: Re: e827091cb1 "block: merge: get the 1st and last bvec via
 helpers" broken

On Sat, 12 Mar 2016 05:02:56 -0900
Kent Overstreet <kent.overstreet@...il.com> wrote:

> Here's the output of the patch below:
> 
> generic/036 11s ...run fstests generic/036 at 2016-03-12 13:58:21
> end 4096 0 ffffea0001d611c0 end2 1024 0 ffffea0001d611c0
> len 1024 offset 0 page ffffea0001d611c0
> KGDB: Waiting for remote debugger
> 
> Your code gives a biovec with bv_len of 4096, the old code gives a biovec with
> bv_len of 1024 (and then we dump every biovec, we see that the bio had only a
> single biovec that did indeed have bv_len == 1024).

I guess we shouldn't have optimized for the case of non-cloned bio, could you
try the following patch?

--
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1e7248f..4abc129 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -267,11 +267,6 @@ static inline void bio_get_last_bvec(struct bio *bio, struct bio_vec *bv)
 	struct bvec_iter iter = bio->bi_iter;
 	int idx;
 
-	if (!bio_flagged(bio, BIO_CLONED)) {
-		*bv = bio->bi_io_vec[bio->bi_vcnt - 1];
-		return;
-	}
-
 	if (unlikely(!bio_multiple_segments(bio))) {
 		*bv = bio_iovec(bio);
 		return;

Thanks,
Ming

> 
> bio_get_last_bvec() is broken, please revert your patch for v4.5.
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 261353166d..231aec4fa3 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -318,6 +318,34 @@ static int blk_phys_contig_segment(struct request_queue *q, struct bio *bio,
>  	bio_get_last_bvec(bio, &end_bv);
>  	bio_get_first_bvec(nxt, &nxt_bv);
>  
> +	{
> +		struct bvec_iter iter;
> +		struct bio_vec end2, bv;
> +
> +		bio_for_each_segment(end2, bio, iter)
> +			if (end2.bv_len == iter.bi_size)
> +				break;
> +
> +		if (end_bv.bv_len	!= end2.bv_len ||
> +		    end_bv.bv_offset	!= end2.bv_offset ||
> +		    end_bv.bv_page	!= end2.bv_page) {
> +			printk(KERN_ERR "end %u %u %p end2 %u %u %p\n",
> +			      end_bv.bv_len,
> +			      end_bv.bv_offset,
> +			      end_bv.bv_page,
> +			      end2.bv_len,
> +			      end2.bv_offset,
> +			      end2.bv_page);
> +
> +			bio_for_each_segment(bv, bio, iter)
> +				printk(KERN_ERR "len %u offset %u page %p\n",
> +				       bv.bv_len,
> +				       bv.bv_offset,
> +				       bv.bv_page);
> +			BUG();
> +		}
> +	}
> +
>  	if (!BIOVEC_PHYS_MERGEABLE(&end_bv, &nxt_bv))
>  		return 0;
>