[PATCH] null_blk: set dma alignment to logical block size

[PATCH] null_blk: set dma alignment to logical block size

[Hans Holmberg]

This driver assumes that bio vectors are memory aligned to the logical
block size, so set the queue limit to reflect that.

Unless we set up the limit based on the logical block size, we will go
out of page bounds in copy_to_nullb / copy_from_nullb.

Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
---

A fixes tag would be in order, but I have not figured out exactly when
this became a problem.

 drivers/block/null_blk/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index f982027e8c85..0ee55f889cfd 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1949,6 +1949,7 @@ static int null_add_dev(struct nullb_device *dev)
 		.logical_block_size	= dev->blocksize,
 		.physical_block_size	= dev->blocksize,
 		.max_hw_sectors		= dev->max_sectors,
+		.dma_alignment		= dev->blocksize - 1,
 	};
 
 	struct nullb *nullb;
-- 
2.34.1

Re: [PATCH] null_blk: set dma alignment to logical block size

[Christoph Hellwig]

On Wed, Oct 29, 2025 at 02:39:56PM +0100, Hans Holmberg wrote:
> This driver assumes that bio vectors are memory aligned to the logical
> block size, so set the queue limit to reflect that.
> 
> Unless we set up the limit based on the logical block size, we will go
> out of page bounds in copy_to_nullb / copy_from_nullb.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> ---
> 
> A fixes tag would be in order, but I have not figured out exactly when
> this became a problem.

In theory probably since day 1, and in practice since direct I/O allowed
less than lba size memory alignment:

Fixes: bf8d08532bc1 ("iomap: add support for dma aligned direct-io")
Fixes: b1a000d3b8ec ("block: relax direct io memory alignment")

Re: [PATCH] null_blk: set dma alignment to logical block size

[Christoph Hellwig]

On Wed, Oct 29, 2025 at 02:51:03PM +0100, Christoph Hellwig wrote:
> On Wed, Oct 29, 2025 at 02:39:56PM +0100, Hans Holmberg wrote:
> > This driver assumes that bio vectors are memory aligned to the logical
> > block size, so set the queue limit to reflect that.
> > 
> > Unless we set up the limit based on the logical block size, we will go
> > out of page bounds in copy_to_nullb / copy_from_nullb.
> > 
> > Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> > ---
> > 
> > A fixes tag would be in order, but I have not figured out exactly when
> > this became a problem.
> 
> In theory probably since day 1, and in practice since direct I/O allowed
> less than lba size memory alignment:
> 
> Fixes: bf8d08532bc1 ("iomap: add support for dma aligned direct-io")
> Fixes: b1a000d3b8ec ("block: relax direct io memory alignment")

Oh, and the patch itself of course looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] null_blk: set dma alignment to logical block size

[Keith Busch]

On Wed, Oct 29, 2025 at 02:39:56PM +0100, Hans Holmberg wrote:
> This driver assumes that bio vectors are memory aligned to the logical
> block size, so set the queue limit to reflect that.
> 
> Unless we set up the limit based on the logical block size, we will go
> out of page bounds in copy_to_nullb / copy_from_nullb.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> ---
> 
> A fixes tag would be in order, but I have not figured out exactly when
> this became a problem.

Sorry! This is from the relaxed memory address alignment changes I've
been making to reduce the need for bounce buffers.

> @@ -1949,6 +1949,7 @@ static int null_add_dev(struct nullb_device *dev)
>  		.logical_block_size	= dev->blocksize,
>  		.physical_block_size	= dev->blocksize,
>  		.max_hw_sectors		= dev->max_sectors,
> +		.dma_alignment		= dev->blocksize - 1,
>  	};

It looks like null_blk only needs (SECTOR_SIZE - 1). All the data copies
work in sector_t units and SECTOR_SIZE granularity, so does dma
alignment really need to match the blocksize if it's, for example, 4k?
And if not, since null_blk didn't set dma_alignment before, it should
have been defaulting to 511 already.

Re: [PATCH] null_blk: set dma alignment to logical block size

[Hans Holmberg]

On 29/10/2025 15:12, Keith Busch wrote:
> On Wed, Oct 29, 2025 at 02:39:56PM +0100, Hans Holmberg wrote:
>> This driver assumes that bio vectors are memory aligned to the logical
>> block size, so set the queue limit to reflect that.
>>
>> Unless we set up the limit based on the logical block size, we will go
>> out of page bounds in copy_to_nullb / copy_from_nullb.
>>
>> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
>> ---
>>
>> A fixes tag would be in order, but I have not figured out exactly when
>> this became a problem.
> 
> Sorry! This is from the relaxed memory address alignment changes I've
> been making to reduce the need for bounce buffers.
> 
>> @@ -1949,6 +1949,7 @@ static int null_add_dev(struct nullb_device *dev)
>>  		.logical_block_size	= dev->blocksize,
>>  		.physical_block_size	= dev->blocksize,
>>  		.max_hw_sectors		= dev->max_sectors,
>> +		.dma_alignment		= dev->blocksize - 1,
>>  	};
> 
> It looks like null_blk only needs (SECTOR_SIZE - 1). All the data copies
> work in sector_t units and SECTOR_SIZE granularity, so does dma
> alignment really need to match the blocksize if it's, for example, 4k?
> And if not, since null_blk didn't set dma_alignment before, it should
> have been defaulting to 511 already.
> 

Hmm, the data is actually copied nullb->dev->blocksize sized chunks

see copy_to_nullb:

temp = min_t(size_t, nullb->dev->blocksize, n - count);
..
memcpy_page(t_page->page, offset, source, off + count, temp);

Just to verify that nullblk was missbehaving I enabled debugging
for the copy and hit a bug_on in memcpy_page:

VM_BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);

(This was a 4k block size nullblk with the default 511 byte dma limit)





 

Re: [PATCH] null_blk: set dma alignment to logical block size

[Hans Holmberg]

On 29/10/2025 16:32, Hans Holmberg wrote:
> On 29/10/2025 15:12, Keith Busch wrote:
>> On Wed, Oct 29, 2025 at 02:39:56PM +0100, Hans Holmberg wrote:
>>> This driver assumes that bio vectors are memory aligned to the logical
>>> block size, so set the queue limit to reflect that.
>>>
>>> Unless we set up the limit based on the logical block size, we will go
>>> out of page bounds in copy_to_nullb / copy_from_nullb.
>>>
>>> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
>>> ---
>>>
>>> A fixes tag would be in order, but I have not figured out exactly when
>>> this became a problem.
>>
>> Sorry! This is from the relaxed memory address alignment changes I've
>> been making to reduce the need for bounce buffers.
>>
>>> @@ -1949,6 +1949,7 @@ static int null_add_dev(struct nullb_device *dev)
>>>  		.logical_block_size	= dev->blocksize,
>>>  		.physical_block_size	= dev->blocksize,
>>>  		.max_hw_sectors		= dev->max_sectors,
>>> +		.dma_alignment		= dev->blocksize - 1,
>>>  	};
>>
>> It looks like null_blk only needs (SECTOR_SIZE - 1). All the data copies
>> work in sector_t units and SECTOR_SIZE granularity, so does dma
>> alignment really need to match the blocksize if it's, for example, 4k?
>> And if not, since null_blk didn't set dma_alignment before, it should
>> have been defaulting to 511 already.
>>
> 
> Hmm, the data is actually copied nullb->dev->blocksize sized chunks
> 
> see copy_to_nullb:
> 
> temp = min_t(size_t, nullb->dev->blocksize, n - count);
> ..
> memcpy_page(t_page->page, offset, source, off + count, temp);
> 
> Just to verify that nullblk was missbehaving I enabled debugging
> for the copy and hit a bug_on in memcpy_page:
> 
> VM_BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> 
> (This was a 4k block size nullblk with the default 511 byte dma limit)
> 

I can reproduce and detect the issue by running xfs/538 in xfstests
with kasan (or the above BUG_ON) enabled.

Re: [PATCH] null_blk: set dma alignment to logical block size

[Keith Busch]

On Wed, Oct 29, 2025 at 03:32:30PM +0000, Hans Holmberg wrote:
> Hmm, the data is actually copied nullb->dev->blocksize sized chunks
> 
> see copy_to_nullb:
> 
> temp = min_t(size_t, nullb->dev->blocksize, n - count);

That's just saying blocksize is the largest it will transfer. It can be
smaller if there's less data than a block size in the bvec.

> ..
> memcpy_page(t_page->page, offset, source, off + count, temp);
> 
> Just to verify that nullblk was missbehaving I enabled debugging
> for the copy and hit a bug_on in memcpy_page:
> 
> VM_BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> 
> (This was a 4k block size nullblk with the default 511 byte dma limit)

Oh, I see. It's that the source buffer with its weird offsets that don't
align can cause a problem. This should fix it:

---
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index f982027e8c858..385fe6523ec12 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1143,6 +1143,7 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source,
 			null_make_cache_space(nullb, PAGE_SIZE);
 
 		offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
+		temp = min_t(size_t, temp, PAGE_SIZE - offset);
 		t_page = null_insert_page(nullb, sector,
 			!null_cache_active(nullb) || is_fua);
 		if (!t_page)
@@ -1172,6 +1173,7 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest,
 		temp = min_t(size_t, nullb->dev->blocksize, n - count);
 
 		offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
+		temp = min_t(size_t, temp, PAGE_SIZE - offset);
 		t_page = null_lookup_page(nullb, sector, false,
 			!null_cache_active(nullb));
 
--

After this, you can set dma_alignment to 1.

Re: [PATCH] null_blk: set dma alignment to logical block size

[Keith Busch]

On Wed, Oct 29, 2025 at 10:06:31AM -0600, Keith Busch wrote:
> 
> After this, you can set dma_alignment to 1.

Oh wait, no, it still can't be smaller than 511 because everything
operates on "SECTOR_SIZE". Which also seems like it shouldn't be
necessary...

Sorry, let's just do your patch. That seems the safest thing for now,
and I can mess with dma alignment limits for a follow up (I'm already
doing something similiar for generic protection information).

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH] null_blk: set dma alignment to logical block size

[Hans Holmberg]

On 29/10/2025 17:16, Keith Busch wrote:
> On Wed, Oct 29, 2025 at 10:06:31AM -0600, Keith Busch wrote:
>>
>> After this, you can set dma_alignment to 1.
> 
> Oh wait, no, it still can't be smaller than 511 because everything
> operates on "SECTOR_SIZE". Which also seems like it shouldn't be
> necessary...
> 
> Sorry, let's just do your patch. That seems the safest thing for now,
> and I can mess with dma alignment limits for a follow up (I'm already
> doing something similiar for generic protection information).
> 

Awesome, it would be great to support smaller granularity in nullblk.
I'd happily test it in my setup.


> Reviewed-by: Keith Busch <kbusch@kernel.org>
> 

Thanks for the review, cheers!

Re: [PATCH] null_blk: set dma alignment to logical block size

[Christoph Hellwig]

Maybe you can resend this with the Fixes tag and a blurb that this path
is exercised by xfstests since  851c4c96db00
("xfs: implement XFS_IOC_DIOINFO in terms of vfs_getattr") and causes
reproducible memory corruption?  Because that might help to get it
included ASAP..

On Wed, Oct 29, 2025 at 02:39:56PM +0100, Hans Holmberg wrote:
> This driver assumes that bio vectors are memory aligned to the logical
> block size, so set the queue limit to reflect that.
> 
> Unless we set up the limit based on the logical block size, we will go
> out of page bounds in copy_to_nullb / copy_from_nullb.
> 
> Signed-off-by: Hans Holmberg <hans.holmberg@wdc.com>
> ---
> 
> A fixes tag would be in order, but I have not figured out exactly when
> this became a problem.
> 
>  drivers/block/null_blk/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index f982027e8c85..0ee55f889cfd 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1949,6 +1949,7 @@ static int null_add_dev(struct nullb_device *dev)
>  		.logical_block_size	= dev->blocksize,
>  		.physical_block_size	= dev->blocksize,
>  		.max_hw_sectors		= dev->max_sectors,
> +		.dma_alignment		= dev->blocksize - 1,
>  	};
>  
>  	struct nullb *nullb;
> -- 
> 2.34.1
---end quoted text---