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
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
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
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
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
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
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);
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
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
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!
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
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]>
On Tue, Nov 22, 2022 at 3:56 PM David Sterba <[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")
> > 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?
On Tue, Jan 10, 2023 at 01:27:03PM +0100, Alexander Potapenko wrote:
> On Tue, Nov 22, 2022 at 3:56 PM David Sterba <[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")
> > > 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?
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
On Mon, Nov 21, 2022 at 07:53:19PM +0000, Eric Biggers wrote:
> 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]>
>
Jaegeuk, can you please apply this patch?
- Eric
On Mon, Nov 21, 2022 at 12:21 PM Alexander Potapenko <[email protected]> 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]>
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]/)
On Tue, 24 Jan 2023 11:51:30 +0100 Alexander Potapenko <[email protected]> wrote:
> On Mon, Nov 21, 2022 at 12:21 PM Alexander Potapenko <[email protected]> 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]>
>
> 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.
On Tue, Jan 24, 2023 at 10:04 PM Andrew Morton
<[email protected]> wrote:
>
> On Tue, 24 Jan 2023 11:51:30 +0100 Alexander Potapenko <[email protected]> wrote:
>
> > On Mon, Nov 21, 2022 at 12:21 PM Alexander Potapenko <[email protected]> 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]>
> >
> > 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!
Right, makes sense.
On 01/22, Eric Biggers wrote:
> On Mon, Nov 21, 2022 at 07:53:19PM +0000, Eric Biggers wrote:
> > 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]>
> >
>
> Jaegeuk, can you please apply this patch?
Yup, applied.
>
> - Eric