| Message ID | 20221121112134.407362-1-glider@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] fs: ext4: initialize fsdata in pagecache_write() | expand |
On Mon, 21 Nov 2022 12:21:30 +0100 Alexander Potapenko <glider@google.com> 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 <ebiggers@kernel.org> > Fixes: c93d8f885809 ("ext4: add basic fs-verity support") > Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com > Signed-off-by: Alexander Potapenko <glider@google.com> > --- > 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 <ebiggers@google.com> - 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 <glider@google.com> 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!
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);
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 <ebiggers@kernel.org> Fixes: c93d8f885809 ("ext4: add basic fs-verity support") Reported-by: syzbot+9767be679ef5016b6082@syzkaller.appspotmail.com Signed-off-by: Alexander Potapenko <glider@google.com> --- fs/ext4/verity.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)