Skip to contentSkip to conversation

fs: ext4: initialize fsdata in pagecache_write()

PATCH In review Alexander Potapenko 6 participants 20 messages Nov 21, 2022 – Jan 26, 2023

Subsystems (6)
VFS ext4 file system F2FS file system Hfsplus filesystem +2 more subsystems
lore
Share & tools
lore

Alexander Potapenko

author
#0 lore raw

PATCH 1/5 fs: ext4: initialize fsdata in pagecache_write()

When aops->write_begin() does not initialize fsdata, KMSAN reports an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Cc Eric Biggers <[email protected]>
Fixes c93d8f885809 ("ext4: add basic fs-verity support")
Reported-by [email protected]
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/ext4/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext4/verity.c +1 -1
index 3c640bd7ecaeb..30e3b65798b50 100644
@@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
 
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
Signature · 1 line
2.38.1.584.g0f3c55d4c2-goog
author
#1 lore raw

PATCH 2/5 fs: affs: initialize fsdata in affs_truncate()

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/affs/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/affs/file.c +1 -1
index cefa222f7881c..8daeed31e1af9 100644
@@ -880,7 +880,7 @@ affs_truncate(struct inode *inode)
 	if (inode->i_size > AFFS_I(inode)->mmu_private) {
 		struct address_space *mapping = inode->i_mapping;
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		loff_t isize = inode->i_size;
 		int res;
Signature · 1 line
2.38.1.584.g0f3c55d4c2-goog
#2 lore raw

Re: [PATCH 2/5] fs: affs: initialize fsdata in affs_truncate()

Quoted 7 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Are you sure that is the correct Fixes commit? What about commit f2b6a16eb8f5 ("fs: affs convert to new aops")?

- Eric

author
#3 lore raw

Re: [PATCH 2/5] fs: affs: initialize fsdata in affs_truncate()

Quoted 5 lines · Eric Biggers
Quoted 3 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Quoted 7 lines
Quoted 4 lines

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Are you sure that is the correct Fixes commit? What about commit f2b6a16eb8f5 ("fs: affs convert to new aops")?

Hmm, indeed, you are right.

Quoted 1 line

- Eric

Signature · 16 lines
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg


_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Acked-by on v1 2/5
#4 lore raw

Re: [PATCH 2/5] fs: affs: initialize fsdata in affs_truncate()

In reply to Alexander Potapenko
Quoted 8 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>

With the fixed Fixes: reference,

Acked-by David Sterba <[email protected]>

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

author
#5 lore raw

Re: [PATCH 2/5] fs: affs: initialize fsdata in affs_truncate()

Quoted 7 lines · David Sterba
Quoted 5 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Quoted 7 lines
Quoted 3 lines
Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>

With the fixed Fixes: reference,

Acked-by David Sterba <[email protected]>

Hi David,

I've noticed that the ext4 counterpart of this patch is in the upstream tree already, whereas the affs, f2fs, hfs and hfsplus versions are not.
Are they picked via a different tree?

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

#6 lore raw

Re: [PATCH 2/5] fs: affs: initialize fsdata in affs_truncate()

Quoted 14 lines · Alexander Potapenko
Quoted 13 lines · David Sterba
Quoted 8 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>

With the fixed Fixes: reference,

Quoted 8 lines
Quoted 1 line
Acked-by David Sterba <[email protected]>

Hi David,

I've noticed that the ext4 counterpart of this patch is in the upstream tree already, whereas the affs, f2fs, hfs and hfsplus versions are not.
Are they picked via a different tree?

I thought it would go in with the rest of the series but it seems that the patches are meant to be merged independently, so I'll do the affs part.

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

#7 lore raw

Re: [PATCH 2/5] fs: affs: initialize fsdata in affs_truncate()

In reply to Alexander Potapenko
Quoted 14 lines · Alexander Potapenko
Quoted 13 lines · David Sterba
Quoted 8 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>

With the fixed Fixes: reference,

Quoted 8 lines
Quoted 1 line
Acked-by David Sterba <[email protected]>

Hi David,

I've noticed that the ext4 counterpart of this patch is in the upstream tree already, whereas the affs, f2fs, hfs and hfsplus versions are not.
Are they picked via a different tree?

Generally each filesystem has its own development tree. All the information is in MAINTAINERS. hfs and hfsplus are unmaintained, though.

Maybe try asking Andrew Morton to apply the hfs and hfsplus patches, and any others that don't get applied, as "maintainer of last resort".

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

author
#8 lore raw

PATCH 3/5 fs: f2fs: initialize fsdata in pagecache_write()

In reply to Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 95ae251fe828 ("f2fs: add fs-verity support")
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/f2fs/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/verity.c +1 -1
index c352fff88a5e6..3f4f3295f1c66 100644
@@ -81,7 +81,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
 
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
Signature · 1 line
2.38.1.584.g0f3c55d4c2-goog
Reviewed-by on v1 3/5
#9 lore raw

Re: [PATCH 3/5] fs: f2fs: initialize fsdata in pagecache_write()

Quoted 17 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 95ae251fe828 ("f2fs: add fs-verity support")
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/f2fs/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/verity.c +0 -0
index c352fff88a5e6..3f4f3295f1c66 100644

fs/f2fs/verity.c static int pagecache_write(struct inode *inode, const void *buf, size_t count, · line 87

Quoted 8 lines
fs/f2fs/verity.c +1 -1
@@ -81,7 +81,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
 
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
Reviewed-by Eric Biggers <[email protected]>

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

#10 lore raw

Re: [PATCH 3/5] fs: f2fs: initialize fsdata in pagecache_write()

Quoted 25 lines · Eric Biggers
Quoted 24 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 95ae251fe828 ("f2fs: add fs-verity support")
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/f2fs/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/verity.c +1 -1
index c352fff88a5e6..3f4f3295f1c66 100644
@@ -81,7 +81,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
Quoted 4 lines

fs/f2fs/verity.c static int pagecache_write(struct inode *inode, const void *buf, size_t count, · line 87

Quoted 1 line
fs/f2fs/verity.c +0 -0
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
Reviewed-by Eric Biggers <[email protected]>

Jaegeuk, can you please apply this patch?

- Eric

maintainer
#11 lore raw

Re: [PATCH 3/5] fs: f2fs: initialize fsdata in pagecache_write()

Quoted 26 lines · Eric Biggers
Quoted 25 lines · Eric Biggers
Quoted 24 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 95ae251fe828 ("f2fs: add fs-verity support")
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/f2fs/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/verity.c +1 -1
index c352fff88a5e6..3f4f3295f1c66 100644
@@ -81,7 +81,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
Quoted 6 lines
Quoted 4 lines

fs/f2fs/verity.c static int pagecache_write(struct inode *inode, const void *buf, size_t count, · line 87

Quoted 1 line
fs/f2fs/verity.c +0 -0
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
Reviewed-by Eric Biggers <[email protected]>

Jaegeuk, can you please apply this patch?

Yup, applied.

Quoted 2 lines

- Eric

_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

author
#12 lore raw

PATCH 4/5 fs: hfs: initialize fsdata in hfs_file_truncate()

In reply to Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/hfs/extent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/hfs/extent.c +1 -1
index 3f7e9bef98743..6d1878b99b305 100644
@@ -486,7 +486,7 @@ void hfs_file_truncate(struct inode *inode)
 		inode->i_size);
 	if (inode->i_size > HFS_I(inode)->phys_size) {
 		struct address_space *mapping = inode->i_mapping;
-		void *fsdata;
+		void *fsdata = NULL;
 		struct page *page;
 
 		/* XXX: Can use generic_cont_expand? */
Signature · 1 line
2.38.1.584.g0f3c55d4c2-goog
author
#13 lore raw

Re: [PATCH 4/5] fs: hfs: initialize fsdata in hfs_file_truncate()

Quoted 9 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>

Dear FS maintainers,

HFS/HFSPLUS are orphaned, can someone take this patch to their tree? Thanks in advance!
(same for "fs: hfsplus: initialize fsdata in hfsplus_file_truncate()":
https://lore.kernel.org/all/[email protected]/)

#14 lore raw

Re: [PATCH 4/5] fs: hfs: initialize fsdata in hfs_file_truncate()

Quoted 11 lines · On Tue, 24 Jan 2023 11:51:30 +0100 Alexander Potapenko <[email protected]>…
Quoted 9 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>
Quoted 6 lines

Dear FS maintainers,

HFS/HFSPLUS are orphaned, can someone take this patch to their tree? Thanks in advance!
(same for "fs: hfsplus: initialize fsdata in hfsplus_file_truncate()":
https://lore.kernel.org/all/[email protected]/)

I grabbed both.

I removed the

	Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

because that might provoke the backport bots to backport this fix across eight years worth of kernels. Before KMSAN existed!

If you intended that this be backported then please let's come up with a more precise Fixes: target and we'll add cc:stable.

author
#15 lore raw

Re: [PATCH 4/5] fs: hfs: initialize fsdata in hfs_file_truncate()

Quoted 21 lines · Andrew Morton
Quoted 11 lines · On Tue, 24 Jan 2023 11:51:30 +0100 Alexander Potapenko <[email protected]>…
Quoted 9 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>
Quoted 6 lines

Dear FS maintainers,

HFS/HFSPLUS are orphaned, can someone take this patch to their tree? Thanks in advance!
(same for "fs: hfsplus: initialize fsdata in hfsplus_file_truncate()":
https://lore.kernel.org/all/[email protected]/)

Quoted 8 lines

I grabbed both.

I removed the

        Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

because that might provoke the backport bots to backport this fix across eight years worth of kernels. Before KMSAN existed!

Right, makes sense.

author
#16 lore raw

PATCH 5/5 fs: hfsplus: initialize fsdata in hfsplus_file_truncate()

In reply to Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN may report an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Suggested-by Eric Biggers <[email protected]>
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/hfsplus/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/hfsplus/extents.c +1 -1
index 721f779b4ec3e..7a542f3dbe502 100644
@@ -554,7 +554,7 @@ void hfsplus_file_truncate(struct inode *inode)
 	if (inode->i_size > hip->phys_size) {
 		struct address_space *mapping = inode->i_mapping;
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		loff_t size = inode->i_size;
 
 		res = hfsplus_write_begin(NULL, mapping, size, 0,
Signature · 1 line
2.38.1.584.g0f3c55d4c2-goog
#17 lore raw
In reply to Alexander Potapenko
Quoted 7 lines · On Mon, 21 Nov 2022 12:21:30 +0100 Alexander Potapenko <[email protected]>…

When aops->write_begin() does not initialize fsdata, KMSAN reports an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

...

I'm assuming that this is not-a-bug, and that these changes are purely workarounds for a KMSAN shortcoming?

If true, this important info should be included in each changelog, please.

If false, please provide a full description of the end-user visible effects of the bug.

Also, it would be helpful to describe why it is not considered practical to teach KMSAN to handle this?

Quoted 11 lines
fs/ext4/verity.c +1 -1
@@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
 
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
#18 lore raw
Quoted 5 lines · Andrew Morton
Quoted 3 lines · On Mon, 21 Nov 2022 12:21:30 +0100 Alexander Potapenko <[email protected]>…

When aops->write_begin() does not initialize fsdata, KMSAN reports an error passing the latter to aops->write_end().

Quoted 7 lines
Quoted 4 lines

Fix this by unconditionally initializing fsdata.

...

I'm assuming that this is not-a-bug, and that these changes are purely workarounds for a KMSAN shortcoming?

It's a weird one. It used to be not-a-bug. Then we changed from std=gnu99 to std=gnu11 or something. And in the intervening years, the C standards ctte decided that passing an uninitialised pointer to a function was UB. So we start by passing a pointer to the pointer to ->write_begin(). Some ->write_begin functions initialise that pointer; others don't. Then we pass the pointer directly to ->write_end. If ->write_begin initialised the pointer, that's fine, and if not, it's UB. Of course the ->write_end doesn't use it if the ->write_begin didn't initialise it, but it's too late because merely calling the function was UB. Thanks, Itanium!

Reviewed-by on v1 1/5
#19 lore raw
In reply to Alexander Potapenko
Quoted 18 lines · Alexander Potapenko

When aops->write_begin() does not initialize fsdata, KMSAN reports an error passing the latter to aops->write_end().

Fix this by unconditionally initializing fsdata.

Cc Eric Biggers <[email protected]>
Fixes c93d8f885809 ("ext4: add basic fs-verity support")
Reported-by [email protected]
Signed-off-by Alexander Potapenko <[email protected]>
---
 fs/ext4/verity.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/ext4/verity.c +0 -0
index 3c640bd7ecaeb..30e3b65798b50 100644

fs/ext4/verity.c static int pagecache_write(struct inode *inode, const void *buf, size_t count, · line 85

Quoted 8 lines
fs/ext4/verity.c +1 -1
@@ -79,7 +79,7 @@ static int pagecache_write(struct inode *inode, const void *buf, size_t count,
 		size_t n = min_t(size_t, count,
 				 PAGE_SIZE - offset_in_page(pos));
 		struct page *page;
-		void *fsdata;
+		void *fsdata = NULL;
 		int res;
 
 		res = aops->write_begin(NULL, mapping, pos, n, &page, &fsdata);
Reviewed-by Eric Biggers <[email protected]>

- Eric