2022-11-21 11:28:01

by Alexander Potapenko

[permalink] [raw]
Subject: [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(-)

diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
index 3c640bd7ecaeb..30e3b65798b50 100644
--- a/fs/ext4/verity.c
+++ b/fs/ext4/verity.c
@@ -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);
--
2.38.1.584.g0f3c55d4c2-goog



2022-11-21 11:28:02

by Alexander Potapenko

[permalink] [raw]
Subject: [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(-)

diff --git a/fs/affs/file.c b/fs/affs/file.c
index cefa222f7881c..8daeed31e1af9 100644
--- a/fs/affs/file.c
+++ b/fs/affs/file.c
@@ -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;

--
2.38.1.584.g0f3c55d4c2-goog


2022-11-21 11:28:13

by Alexander Potapenko

[permalink] [raw]
Subject: [PATCH 3/5] fs: f2fs: initialize fsdata in pagecache_write()

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(-)

diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
index c352fff88a5e6..3f4f3295f1c66 100644
--- a/fs/f2fs/verity.c
+++ b/fs/f2fs/verity.c
@@ -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);
--
2.38.1.584.g0f3c55d4c2-goog


2022-11-21 11:29:02

by Alexander Potapenko

[permalink] [raw]
Subject: [PATCH 4/5] fs: hfs: initialize fsdata in hfs_file_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/hfs/extent.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfs/extent.c b/fs/hfs/extent.c
index 3f7e9bef98743..6d1878b99b305 100644
--- a/fs/hfs/extent.c
+++ b/fs/hfs/extent.c
@@ -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? */
--
2.38.1.584.g0f3c55d4c2-goog


2022-11-21 11:30:38

by Alexander Potapenko

[permalink] [raw]
Subject: [PATCH 5/5] fs: hfsplus: initialize fsdata in hfsplus_file_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/hfsplus/extents.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index 721f779b4ec3e..7a542f3dbe502 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -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,
--
2.38.1.584.g0f3c55d4c2-goog


2022-11-21 19:50:35

by Eric Biggers

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

On Mon, Nov 21, 2022 at 12:21:31PM +0100, Alexander Potapenko wrote:
> 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

2022-11-21 19:51:23

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 1/5] fs: ext4: initialize fsdata in pagecache_write()

On Mon, 21 Nov 2022 12:21:30 +0100 Alexander Potapenko <[email protected]> wrote:

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

> --- a/fs/ext4/verity.c
> +++ b/fs/ext4/verity.c
> @@ -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);


2022-11-21 19:53:40

by Eric Biggers

[permalink] [raw]
Subject: Re: [PATCH 1/5] fs: ext4: initialize fsdata in pagecache_write()

On Mon, Nov 21, 2022 at 12:21:30PM +0100, Alexander Potapenko wrote:
> 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(-)
>
> diff --git a/fs/ext4/verity.c b/fs/ext4/verity.c
> index 3c640bd7ecaeb..30e3b65798b50 100644
> --- a/fs/ext4/verity.c
> +++ b/fs/ext4/verity.c
> @@ -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

2022-11-21 19:56:02

by Eric Biggers

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

On Mon, Nov 21, 2022 at 12:21:32PM +0100, Alexander Potapenko wrote:
> 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(-)
>
> diff --git a/fs/f2fs/verity.c b/fs/f2fs/verity.c
> index c352fff88a5e6..3f4f3295f1c66 100644
> --- a/fs/f2fs/verity.c
> +++ b/fs/f2fs/verity.c
> @@ -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

2022-11-22 03:44:29

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 1/5] fs: ext4: initialize fsdata in pagecache_write()

On Mon, Nov 21, 2022 at 11:48:40AM -0800, Andrew Morton wrote:
> On Mon, 21 Nov 2022 12:21:30 +0100 Alexander Potapenko <[email protected]> wrote:
>
> > 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?

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!

2022-11-22 09:02:36

by Alexander Potapenko

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

On Mon, Nov 21, 2022 at 8:46 PM Eric Biggers <[email protected]> wrote:
>
> On Mon, Nov 21, 2022 at 12:21:31PM +0100, Alexander Potapenko wrote:
> > 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")?

Hmm, indeed, you are right.

> - Eric



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

2022-11-22 14:59:58

by David Sterba

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

On Mon, Nov 21, 2022 at 12:21:31PM +0100, Alexander Potapenko wrote:
> 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]>