Skip to contentSkip to conversation

ext4: honor NULL returns from __filemap_get_folio()

PATCH In review Naman Jain 3 participants 4 messages Nov 2 Stable branch

lore
Share & tools
lore

Naman Jain

author
#0 lore raw

PATCH 1/1 ext4: honor NULL returns from __filemap_get_folio()

The bitmap and buddy folio conversions assume that __filemap_get_folio() returns error pointers. In Linux 6.1, however, it returns NULL both when a non-creating lookup misses and when folio creation fails.

A cache miss therefore passes the IS_ERR() check and reaches folio_test_uptodate(NULL) in ext4_mb_load_buddy_gfp(). Creating-lookup failures can also dereference NULL in its slow paths and in ext4_mb_get_buddy_page_lock().

Restore NULL checks at all six lookup sites and return -ENOMEM when a creating lookup fails. Preserve the folio locking and migration checks without changing the global filemap API.

Fixes d34e4fd4e73f ("ext4: convert bd_bitmap_page to bd_bitmap_folio")
Fixes 862483175d3d ("ext4: convert bd_buddy_page to bd_buddy_folio")
---
 fs/ext4/mballoc.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
fs/ext4/mballoc.c +14 -14
index e77d56c533c0b..d0b276ce9401c 100644
@@ -1375,8 +1375,8 @@ static int ext4_mb_get_buddy_page_lock(struct super_block *sb,
 	poff = block % blocks_per_page;
 	folio = __filemap_get_folio(inode->i_mapping, pnum,
 			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
-	if (IS_ERR(folio))
-		return PTR_ERR(folio);
+	if (!folio)
+		return -ENOMEM;
 	BUG_ON(folio->mapping != inode->i_mapping);
 	e4b->bd_bitmap_folio = folio;
 	e4b->bd_bitmap = folio_address(folio) + (poff * sb->s_blocksize);
@@ -1389,8 +1389,8 @@ static int ext4_mb_get_buddy_page_lock(struct super_block *sb,
 	/* blocks_per_page == 1, hence we need another page for the buddy */
 	folio = __filemap_get_folio(inode->i_mapping, block + 1,
 			FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
-	if (IS_ERR(folio))
-		return PTR_ERR(folio);
+	if (!folio)
+		return -ENOMEM;
 	BUG_ON(folio->mapping != inode->i_mapping);
 	e4b->bd_buddy_folio = folio;
 	return 0;
@@ -1533,7 +1533,7 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group,
 
 	/* Avoid locking the folio in the fast path ... */
 	folio = __filemap_get_folio(inode->i_mapping, pnum, FGP_ACCESSED, 0);
-	if (IS_ERR(folio) || !folio_test_uptodate(folio) || folio_test_locked(folio)) {
+	if (!folio || !folio_test_uptodate(folio) || folio_test_locked(folio)) {
 		/*
 		 * folio_test_locked is employed to detect ongoing folio
 		 * migrations, since concurrent migrations can lead to
@@ -1543,11 +1543,11 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group,
 		 * try to get the folio with lock in both cases to avoid
 		 * concurrency.
 		 */
-		if (!IS_ERR(folio))
+		if (folio)
 			folio_put(folio);
 		folio = __filemap_get_folio(inode->i_mapping, pnum,
 				FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
-		if (!IS_ERR(folio)) {
+		if (folio) {
 			if (WARN_RATELIMIT(folio->mapping != inode->i_mapping,
 	"ext4: bitmap's mapping != inode->i_mapping\n")) {
 				/* should never happen */
@@ -1567,8 +1567,8 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group,
 			folio_unlock(folio);
 		}
 	}
-	if (IS_ERR(folio)) {
-		ret = PTR_ERR(folio);
+	if (!folio) {
+		ret = -ENOMEM;
 		goto err;
 	}
 	if (!folio_test_uptodate(folio)) {
@@ -1585,12 +1585,12 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group,
 	poff = block % blocks_per_page;
 
 	folio = __filemap_get_folio(inode->i_mapping, pnum, FGP_ACCESSED, 0);
-	if (IS_ERR(folio) || !folio_test_uptodate(folio) || folio_test_locked(folio)) {
-		if (!IS_ERR(folio))
+	if (!folio || !folio_test_uptodate(folio) || folio_test_locked(folio)) {
+		if (folio)
 			folio_put(folio);
 		folio = __filemap_get_folio(inode->i_mapping, pnum,
 				FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
-		if (!IS_ERR(folio)) {
+		if (folio) {
 			if (WARN_RATELIMIT(folio->mapping != inode->i_mapping,
 	"ext4: buddy bitmap's mapping != inode->i_mapping\n")) {
 				/* should never happen */
@@ -1609,8 +1609,8 @@ ext4_mb_load_buddy_gfp(struct super_block *sb, ext4_group_t group,
 			folio_unlock(folio);
 		}
 	}
-	if (IS_ERR(folio)) {
-		ret = PTR_ERR(folio);
+	if (!folio) {
+		ret = -ENOMEM;
 		goto err;
 	}
 	if (!folio_test_uptodate(folio)) {
Signature · 1 line
2.43.0
#1 lore raw
Sashiko has reviewed this patch and found no issues.
Quoted 10 lines

The bitmap and buddy folio conversions assume that __filemap_get_folio() returns error pointers. In Linux 6.1, however, it returns NULL both when a non-creating lookup misses and when folio creation fails.

A cache miss therefore passes the IS_ERR() check and reaches folio_test_uptodate(NULL) in ext4_mb_load_buddy_gfp(). Creating-lookup failures can also dereference NULL in its slow paths and in ext4_mb_get_buddy_page_lock().

Quoted 8 lines

Restore NULL checks at all six lookup sites and return -ENOMEM when a creating lookup fails. Preserve the folio locking and migration checks without changing the global filemap API.

Fixes d34e4fd4e73f ("ext4: convert bd_bitmap_page to bd_bitmap_folio")
Fixes 862483175d3d ("ext4: convert bd_buddy_page to bd_buddy_folio")

Sashiko has reviewed this patch and found no issues.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#2 lore raw
In reply to Naman Jain
Quoted 10 lines · Naman Jain

The bitmap and buddy folio conversions assume that __filemap_get_folio() returns error pointers. In Linux 6.1, however, it returns NULL both when a non-creating lookup misses and when folio creation fails.

A cache miss therefore passes the IS_ERR() check and reaches folio_test_uptodate(NULL) in ext4_mb_load_buddy_gfp(). Creating-lookup failures can also dereference NULL in its slow paths and in ext4_mb_get_buddy_page_lock().

Quoted 8 lines

Restore NULL checks at all six lookup sites and return -ENOMEM when a creating lookup fails. Preserve the folio locking and migration checks without changing the global filemap API.

Fixes d34e4fd4e73f ("ext4: convert bd_bitmap_page to bd_bitmap_folio")
Fixes 862483175d3d ("ext4: convert bd_buddy_page to bd_buddy_folio")

Elliot beat you to it:

https://lore.kernel.org/linux-ext4/[email protected]/

Care to drop a R-b on his series?

author
#3 lore raw
Quoted 20 lines · Matthew Wilcox
Quoted 10 lines · Naman Jain

The bitmap and buddy folio conversions assume that __filemap_get_folio() returns error pointers. In Linux 6.1, however, it returns NULL both when a non-creating lookup misses and when folio creation fails.

A cache miss therefore passes the IS_ERR() check and reaches folio_test_uptodate(NULL) in ext4_mb_load_buddy_gfp(). Creating-lookup failures can also dereference NULL in its slow paths and in ext4_mb_get_buddy_page_lock().

Quoted 8 lines

Restore NULL checks at all six lookup sites and return -ENOMEM when a creating lookup fails. Preserve the folio locking and migration checks without changing the global filemap API.

Fixes d34e4fd4e73f ("ext4: convert bd_bitmap_page to bd_bitmap_folio")
Fixes 862483175d3d ("ext4: convert bd_buddy_page to bd_buddy_folio")
Quoted 5 lines

Elliot beat you to it:

https://lore.kernel.org/linux-ext4/[email protected]/

Care to drop a R-b on his series?

Done.
I actually checked LKML in the morning for this :)

Regards,
Naman