Re: [PATCH 3/3] xfs: alignment check bio buffers

[Dave Chinner <david@fromorbit.com>]

On Wed, Aug 21, 2019 at 04:29:45PM -0700, Christoph Hellwig wrote:
> On Wed, Aug 21, 2019 at 06:38:20PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Add memory buffer alignment validation checks to bios built in XFS
> > to catch bugs that will result in silent data corruption in block
> > drivers that cannot handle unaligned memory buffers but don't
> > validate the incoming buffer alignment is correct.
> > 
> > Known drivers with these issues are xenblk, brd and pmem.
> > 
> > Despite there being nothing XFS specific to xfs_bio_add_page(), this
> > function was created to do the required validation because the block
> > layer developers that keep telling us that is not possible to
> > validate buffer alignment in bio_add_page(), and even if it was
> > possible it would be too much overhead to do at runtime.
> 
> I really don't think we should life this to XFS, but instead fix it
> in the block layer.  And that is not only because I have a pending
> series lifting bits you are touching to the block layer..

I agree, but....

> 
> > +int
> > +xfs_bio_add_page(
> > +	struct bio	*bio,
> > +	struct page	*page,
> > +	unsigned int	len,
> > +	unsigned int	offset)
> > +{
> > +	struct request_queue	*q = bio->bi_disk->queue;
> > +	bool		same_page = false;
> > +
> > +	if (WARN_ON_ONCE(!blk_rq_aligned(q, len, offset)))
> > +		return -EIO;
> > +
> > +	if (!__bio_try_merge_page(bio, page, len, offset, &same_page)) {
> > +		if (bio_full(bio, len))
> > +			return 0;
> > +		__bio_add_page(bio, page, len, offset);
> > +	}
> > +	return len;
> 
> I know Jens disagree, but with the amount of bugs we've been hitting
> thangs to slub (and I'm pretty sure we have a more hiding outside of
> XFS) I think we need to add the blk_rq_aligned check to bio_add_page.

... I'm not prepared to fight this battle to get this initial fix
into the code. Get the fix merged, then we can 

> Note that all current callers of bio_add_page can only really check
> for the return value != the added len anyway, so it is not going to
> make anything worse.

It does make things worse - it turns multi-bio chaining loops like
the one xfs_rw_bdev() into an endless loop as they don't make
progress - they just keep allocating a new bio and retrying the same
badly aligned buffer and failing. So if we want an alignment failure
to error out, callers need to handle the failure, not treat it like
a full bio.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com