linux-kernel - RE: [Patch v2] block: return the correct bvec when checking for gaps

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] [day] [month] [year] [list]

Date:   Mon, 7 Jun 2021 18:00:16 +0000
From:   Long Li <longli@...rosoft.com>
To:     Christoph Hellwig <hch@...radead.org>,
        "longli@...uxonhyperv.com" <longli@...uxonhyperv.com>
CC:     "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        Jens Axboe <axboe@...nel.dk>,
        Johannes Thumshirn <johannes.thumshirn@....com>,
        Pavel Begunkov <asml.silence@...il.com>,
        Ming Lei <ming.lei@...hat.com>, Tejun Heo <tj@...nel.org>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Jeffle Xu <jefflexu@...ux.alibaba.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [Patch v2] block: return the correct bvec when checking for gaps

> Subject: Re: [Patch v2] block: return the correct bvec when checking for gaps
> 
> On Fri, Jun 04, 2021 at 04:37:19PM -0700, longli@...uxonhyperv.com wrote:
> > From: Long Li <longli@...rosoft.com>
> >
> > After commit 07173c3ec276 ("block: enable multipage bvecs"), a bvec
> > can have multiple pages. But bio_will_gap() still assumes one page
> > bvec while checking for merging. If the pages in the bvec go across
> > the seg_boundary_mask, this check for merging can potentially succeed
> > if only the 1st page is tested, and can fail if all the pages are tested.
> >
> > Later, when SCSI builds the SG list the same check for merging is done
> > in
> > __blk_segment_map_sg_merge() with all the pages in the bvec tested.
> > This time the check may fail if the pages in bvec go across the
> > seg_boundary_mask (but tested okay in bio_will_gap() earlier, so those
> > BIOs were merged). If this check fails, we end up with a broken SG
> > list for drivers assuming the SG list not having offsets in intermediate pages.
> > This results in incorrect pages written to the disk.
> >
> > Fix this by returning the multi-page bvec when testing gaps for merging.
> >
> > Cc: Jens Axboe <axboe@...nel.dk>
> > Cc: Johannes Thumshirn <johannes.thumshirn@....com>
> > Cc: Pavel Begunkov <asml.silence@...il.com>
> > Cc: Ming Lei <ming.lei@...hat.com>
> > Cc: Tejun Heo <tj@...nel.org>
> > Cc: "Matthew Wilcox (Oracle)" <willy@...radead.org>
> > Cc: Jeffle Xu <jefflexu@...ux.alibaba.com>
> > Cc: linux-kernel@...r.kernel.org
> > Cc: stable@...r.kernel.org
> > Fixes: 07173c3ec276 ("block: enable multipage bvecs")
> > Signed-off-by: Long Li <longli@...rosoft.com>
> > ---
> > Change from v1: add commit details on how data corruption happens
> >
> >  include/linux/bio.h | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/bio.h b/include/linux/bio.h index
> > a0b4cfdf62a4..6b2f609ccfbf 100644
> > --- a/include/linux/bio.h
> > +++ b/include/linux/bio.h
> > @@ -44,9 +44,6 @@ static inline unsigned int bio_max_segs(unsigned int
> nr_segs)
> >  #define bio_offset(bio)		bio_iter_offset((bio), (bio)->bi_iter)
> >  #define bio_iovec(bio)		bio_iter_iovec((bio), (bio)->bi_iter)
> >
> > -#define bio_multiple_segments(bio)				\
> > -	((bio)->bi_iter.bi_size != bio_iovec(bio).bv_len)
> > -
> >  #define bvec_iter_sectors(iter)	((iter).bi_size >> 9)
> >  #define bvec_iter_end_sector(iter) ((iter).bi_sector +
> > bvec_iter_sectors((iter)))
> >
> > @@ -271,7 +268,7 @@ static inline void bio_clear_flag(struct bio *bio,
> > unsigned int bit)
> >
> >  static inline void bio_get_first_bvec(struct bio *bio, struct bio_vec
> > *bv)  {
> > -	*bv = bio_iovec(bio);
> > +	*bv = mp_bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
> >  }
> >
> >  static inline void bio_get_last_bvec(struct bio *bio, struct bio_vec
> > *bv) @@ -279,10 +276,10 @@ static inline void bio_get_last_bvec(struct
> bio *bio, struct bio_vec *bv)
> >  	struct bvec_iter iter = bio->bi_iter;
> >  	int idx;
> >
> > -	if (unlikely(!bio_multiple_segments(bio))) {
> > -		*bv = bio_iovec(bio);
> > +	/* this bio has only one bvec */
> > +	*bv = mp_bvec_iter_bvec(bio->bi_io_vec, bio->bi_iter);
> > +	if (bv->bv_len == bio->bi_iter.bi_size)
> >  		return;
> 
> Nit: I'd move the comment a bit as the current placement confused me at
> first.  Also maybe use bio_get_first_bvec here to make it even more
> obvious:
> 
> 	bio_get_first_bvec(bio, bv);
> 	if (bv->bv_len == bio->bi_iter.bi_size)
> 		return;		/* this bio only has a single bvec */

Thanks! I'll send v3 with those changes.