[v2] md/md-bitmap: fix writing non bitmap pages

Commit Message

Ofir Gal June 7, 2024, 7:27 a.m. UTC

__write_sb_page() rounds up the io size to the optimal io size if it
doesn't exceed the data offset, but it doesn't check the final size
exceeds the bitmap length.

For example:
page count      - 1
page size       - 4K
data offset     - 1M
optimal io size - 256K

The final io size would be 256K (64 pages) but md_bitmap_storage_alloc()
allocated 1 page, the IO would write 1 valid page and 63 pages that
happens to be allocated afterwards. This leaks memory to the raid device
superblock.

This issue caused a data transfer failure in nvme-tcp. The network
drivers checks the first page of an IO with sendpage_ok(), it returns
true if the page isn't a slabpage and refcount >= 1. If the page
!sendpage_ok() the network driver disables MSG_SPLICE_PAGES.

As of now the network layer assumes all the pages of the IO are
sendpage_ok() when MSG_SPLICE_PAGES is on.

The bitmap pages aren't slab pages, the first page of the IO is
sendpage_ok(), but the additional pages that happens to be allocated
after the bitmap pages might be !sendpage_ok(). That cause
skb_splice_from_iter() to stop the data transfer, in the case below it
hangs 'mdadm --create'.

The bug is reproducible, in order to reproduce we need nvme-over-tcp
controllers with optimal IO size bigger than PAGE_SIZE. Creating a raid
with bitmap over those devices reproduces the bug.

In order to simulate large optimal IO size you can use dm-stripe with a
single device.
Script to reproduce the issue on top of brd devices using dm-stripe is
attached below (will be added to blktest).

I have added some logs to test the theory:
...
md: created bitmap (1 pages) for device md127
__write_sb_page before md_super_write offset: 16, size: 262144. pfn: 0x53ee
=== __write_sb_page before md_super_write. logging pages ===
pfn: 0x53ee, slab: 0 <-- the only page that allocated for the bitmap
pfn: 0x53ef, slab: 1
pfn: 0x53f0, slab: 0
pfn: 0x53f1, slab: 0
pfn: 0x53f2, slab: 0
pfn: 0x53f3, slab: 1
...
nvme_tcp: sendpage_ok - pfn: 0x53ee, len: 262144, offset: 0
skbuff: before sendpage_ok() - pfn: 0x53ee
skbuff: before sendpage_ok() - pfn: 0x53ef
WARNING at net/core/skbuff.c:6848 skb_splice_from_iter+0x142/0x450
skbuff: !sendpage_ok - pfn: 0x53ef. is_slab: 1, page_count: 1
...

Reviewed-by: Christoph Hellwig <[e-mail]>
Signed-off-by: Ofir Gal <[e-mail]>
---
Changelog:
v2, split bitmap_limit line after "<<", removed pointless case, add
    Christoph Hellwig review tag.

 drivers/md/md-bitmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Song Liu June 12, 2024, 4:38 p.m. UTC | #1

On Fri, Jun 7, 2024 at 12:28 AM Ofir Gal <[e-mail]> wrote:
>
> __write_sb_page() rounds up the io size to the optimal io size if it
> doesn't exceed the data offset, but it doesn't check the final size
> exceeds the bitmap length.
>
> For example:
> page count      - 1
> page size       - 4K
> data offset     - 1M
> optimal io size - 256K
>
> The final io size would be 256K (64 pages) but md_bitmap_storage_alloc()
> allocated 1 page, the IO would write 1 valid page and 63 pages that
> happens to be allocated afterwards. This leaks memory to the raid device
> superblock.
>
> This issue caused a data transfer failure in nvme-tcp. The network
> drivers checks the first page of an IO with sendpage_ok(), it returns
> true if the page isn't a slabpage and refcount >= 1. If the page
> !sendpage_ok() the network driver disables MSG_SPLICE_PAGES.
>
> As of now the network layer assumes all the pages of the IO are
> sendpage_ok() when MSG_SPLICE_PAGES is on.
>
> The bitmap pages aren't slab pages, the first page of the IO is
> sendpage_ok(), but the additional pages that happens to be allocated
> after the bitmap pages might be !sendpage_ok(). That cause
> skb_splice_from_iter() to stop the data transfer, in the case below it
> hangs 'mdadm --create'.
>
> The bug is reproducible, in order to reproduce we need nvme-over-tcp
> controllers with optimal IO size bigger than PAGE_SIZE. Creating a raid
> with bitmap over those devices reproduces the bug.
>
> In order to simulate large optimal IO size you can use dm-stripe with a
> single device.
> Script to reproduce the issue on top of brd devices using dm-stripe is
> attached below (will be added to blktest).
>
> I have added some logs to test the theory:
> ...
> md: created bitmap (1 pages) for device md127
> __write_sb_page before md_super_write offset: 16, size: 262144. pfn: 0x53ee
> === __write_sb_page before md_super_write. logging pages ===
> pfn: 0x53ee, slab: 0 <-- the only page that allocated for the bitmap
> pfn: 0x53ef, slab: 1
> pfn: 0x53f0, slab: 0
> pfn: 0x53f1, slab: 0
> pfn: 0x53f2, slab: 0
> pfn: 0x53f3, slab: 1
> ...
> nvme_tcp: sendpage_ok - pfn: 0x53ee, len: 262144, offset: 0
> skbuff: before sendpage_ok() - pfn: 0x53ee
> skbuff: before sendpage_ok() - pfn: 0x53ef
> WARNING at net/core/skbuff.c:6848 skb_splice_from_iter+0x142/0x450
> skbuff: !sendpage_ok - pfn: 0x53ef. is_slab: 1, page_count: 1
> ...
>
> Reviewed-by: Christoph Hellwig <[e-mail]>
> Signed-off-by: Ofir Gal <[e-mail]>

Applied to md-6.11. Thanks!

Song

Patch

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 0a2d37eb38ef..08232d8dc815 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -227,6 +227,8 @@  static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap,
 	struct block_device *bdev;
 	struct mddev *mddev = bitmap->mddev;
 	struct bitmap_storage *store = &bitmap->storage;
+	unsigned int bitmap_limit = (bitmap->storage.file_pages - pg_index) <<
+		PAGE_SHIFT;
 	loff_t sboff, offset = mddev->bitmap_info.offset;
 	sector_t ps = pg_index * PAGE_SIZE / SECTOR_SIZE;
 	unsigned int size = PAGE_SIZE;
@@ -269,11 +271,9 @@  static int __write_sb_page(struct md_rdev *rdev, struct bitmap *bitmap,
 		if (size == 0)
 			/* bitmap runs in to data */
 			return -EINVAL;
-	} else {
-		/* DATA METADATA BITMAP - no problems */
 	}
 
-	md_super_write(mddev, rdev, sboff + ps, (int) size, page);
+	md_super_write(mddev, rdev, sboff + ps, (int)min(size, bitmap_limit), page);
 	return 0;
 }