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

Christoph Hellwig
2019-08-21T23:29:47Z

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

> +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.

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.