| Message ID | 20240801022739.1199700-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | filemap: Init the newly allocated folio memory to 0 for the filemap | expand |
On Thu, Aug 01, 2024 at 10:27:39AM +0800, Lizhi Xu wrote: > syzbot report KMSAN: uninit-value in pick_link, this is because the > corresponding folio was not found from the mapping, and the memory was > not initialized when allocating a new folio for the filemap. > > To avoid the occurrence of kmsan report uninit-value, initialize the > newly allocated folio memory to 0. NAK. You are papering over the real bug here. That page either * has been returned by find_get_page(), cached, uptodate and with uninitialized contents or * has been returned by successful read_mapping_page() - and left with uninitialized contents or * had inode->i_size in excess of initialized contents. I'd suggest bisecting that.
> > syzbot report KMSAN: uninit-value in pick_link, this is because the > > corresponding folio was not found from the mapping, and the memory was > > not initialized when allocating a new folio for the filemap. > > > > To avoid the occurrence of kmsan report uninit-value, initialize the > > newly allocated folio memory to 0. > > NAK. > > You are papering over the real bug here. Did you see the splat? I think you didn't see that. > > That page either > * has been returned by find_get_page(), cached, uptodate and > with uninitialized contents or > * has been returned by successful read_mapping_page() - and > left with uninitialized contents or > * had inode->i_size in excess of initialized contents. > > I'd suggest bisecting that.
On Thu, Aug 01, 2024 at 01:28:37PM +0800, Lizhi Xu wrote: > > > syzbot report KMSAN: uninit-value in pick_link, this is because the > > > corresponding folio was not found from the mapping, and the memory was > > > not initialized when allocating a new folio for the filemap. > > > > > > To avoid the occurrence of kmsan report uninit-value, initialize the > > > newly allocated folio memory to 0. > > > > NAK. > > > > You are papering over the real bug here. > Did you see the splat? I think you didn't see that. Sigh... It is stepping into uninitialized data in pick_link(), and by the look of traces it's been created by page_get_link(). What page_get_link() does is reading from page cache of symlink; the contents should have come from ->read_folio() (if it's really a symlink on squashfs, that would be squashfs_symlink_read_folio()). Uninit might have happened if * ->read_folio() hadn't been called at all (which is an obvios bug - that's what should've read the symlink contents) or * ->read_folio() had been called, it failed and yet we are still trying to use the resulting page. Again, an obvious bug - if trying to read fails, we should _not_ use the results or leave it in page cache for subsequent callers. * ->read_folio() had been called, claimed to have succeeded and yet it had left something in range 0..inode->i_size-1 uninitialized. Again, a bug, this time in ->read_folio() instance. Your patch is basically "fill the page with zeroes before reading anything into it". It makes KMSAM warning STFU, but it does not fix anything in any of those cases.
On Thu, Aug 01, 2024 at 08:10:16AM +0100, Al Viro wrote: > Your patch is basically "fill the page with zeroes before reading anything > into it". It makes KMSAM warning STFU, but it does not fix anything > in any of those cases. Incidentally, it does that before it goes ahead and calls filemap_read_folio(), with ->read_folio() as callback. So if it does make it STFU, the case 1 (->read_folio() not called at all) is out of running. Would be interesting to see if that particular ->read_folio() is returning errors and if so - what errors are actually returned.
On Thu, 1 Aug 2024 08:10:16 +0100, Al Viro wrote: > > > > syzbot report KMSAN: uninit-value in pick_link, this is because the > > > > corresponding folio was not found from the mapping, and the memory was > > > > not initialized when allocating a new folio for the filemap. > > > > > > > > To avoid the occurrence of kmsan report uninit-value, initialize the > > > > newly allocated folio memory to 0. > > > > > > NAK. > > > > > > You are papering over the real bug here. > > Did you see the splat? I think you didn't see that. > > Sigh... It is stepping into uninitialized data in pick_link(), and by > the look of traces it's been created by page_get_link(). > > What page_get_link() does is reading from page cache of symlink; > the contents should have come from ->read_folio() (if it's really > a symlink on squashfs, that would be squashfs_symlink_read_folio()). > > Uninit might have happened if > * ->read_folio() hadn't been called at all (which is an obvios > bug - that's what should've read the symlink contents) or > * ->read_folio() had been called, it failed and yet we are > still trying to use the resulting page. Again, an obvious bug - if > trying to read fails, we should _not_ use the results or leave it > in page cache for subsequent callers. > * ->read_folio() had been called, claimed to have succeeded and > yet it had left something in range 0..inode->i_size-1 uninitialized. > Again, a bug, this time in ->read_folio() instance. read_folio, have you noticed that the file value was passed to read_folio is NULL? fs/namei.c const char *page_get_link(struct dentry *dentry, struct inode *inode ... 5272 read_mapping_page(mapping, 0, NULL); So, Therefore, no matter what, the value of folio will not be initialized by file in read_folio.
On Thu, 1 Aug 2024 16:12:24 +0800, Lizhi Xu wrote: > > > > > syzbot report KMSAN: uninit-value in pick_link, this is because the > > > > > corresponding folio was not found from the mapping, and the memory was > > > > > not initialized when allocating a new folio for the filemap. > > > > > > > > > > To avoid the occurrence of kmsan report uninit-value, initialize the > > > > > newly allocated folio memory to 0. > > > > > > > > NAK. > > > > > > > > You are papering over the real bug here. > > > Did you see the splat? I think you didn't see that. > > > > Sigh... It is stepping into uninitialized data in pick_link(), and by > > the look of traces it's been created by page_get_link(). > > > > What page_get_link() does is reading from page cache of symlink; > > the contents should have come from ->read_folio() (if it's really > > a symlink on squashfs, that would be squashfs_symlink_read_folio()). > > > > Uninit might have happened if > > * ->read_folio() hadn't been called at all (which is an obvios > > bug - that's what should've read the symlink contents) or > > * ->read_folio() had been called, it failed and yet we are > > still trying to use the resulting page. Again, an obvious bug - if > > trying to read fails, we should _not_ use the results or leave it > > in page cache for subsequent callers. > > * ->read_folio() had been called, claimed to have succeeded and > > yet it had left something in range 0..inode->i_size-1 uninitialized. > > Again, a bug, this time in ->read_folio() instance. > read_folio, have you noticed that the file value was passed to read_folio is NULL? > fs/namei.c > const char *page_get_link(struct dentry *dentry, struct inode *inode > ... > 5272 read_mapping_page(mapping, 0, NULL); > > So, Therefore, no matter what, the value of folio will not be initialized by file > in read_folio. Oh, in read_folio, it will use mapping->host to init folio, I will research why not init in do_read_cache_folio. -- Lizhi
On Thu, Aug 01, 2024 at 04:12:24PM +0800, Lizhi Xu wrote: > > * ->read_folio() had been called, claimed to have succeeded and > > yet it had left something in range 0..inode->i_size-1 uninitialized. > > Again, a bug, this time in ->read_folio() instance. > read_folio, have you noticed that the file value was passed to read_folio is NULL? > fs/namei.c > const char *page_get_link(struct dentry *dentry, struct inode *inode > ... > 5272 read_mapping_page(mapping, 0, NULL); > > So, Therefore, no matter what, the value of folio will not be initialized by file > in read_folio. What does struct file have to do with anything? What it asks is the first page of the address space of inode in question. file argument of ->read_folio() is not how an instance determines which filesystem object it's dealing with. _That_ is determined by the address space (mapping) the folio had been attached to. For some filesystems that is not enough - they need an information established at open() time. Those ->read_folio() instances can pick such stuff from the file argument - and those obviously cannot be used with page_get_link(), since for symlinks there's no opened files, etc. Most of the instances do not use the 'file' argument. In particular, squashfs_symlink_read_folio() doesn't even look at it. It would probably be less confusing if the arguments of ->read_folio() went in the opposite order, but that's a separate story. In any case, "which filesystem object" is determined by folio->mapping, "which offset in that filesystem object" comes from folio_pos(folio), not that it realistically could be anything other than 0 in case of a symlink (they can't be more than 4Kb long, so the first page will cover the entire thing).
diff --git a/mm/filemap.c b/mm/filemap.c index 657bcd887fdb..1b22eab691e8 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3753,6 +3753,11 @@ static struct folio *do_read_cache_folio(struct address_space *mapping, folio = filemap_alloc_folio(gfp, 0); if (!folio) return ERR_PTR(-ENOMEM); + + void *kaddr = kmap_local_folio(folio, 0); + memset(kaddr, 0, folio_size(folio)); + kunmap_local(kaddr); + err = filemap_add_folio(mapping, folio, index, gfp); if (unlikely(err)) { folio_put(folio);
syzbot report KMSAN: uninit-value in pick_link, this is because the corresponding folio was not found from the mapping, and the memory was not initialized when allocating a new folio for the filemap. To avoid the occurrence of kmsan report uninit-value, initialize the newly allocated folio memory to 0. Reported-and-tested-by: syzbot+24ac24ff58dc5b0d26b9@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=24ac24ff58dc5b0d26b9 Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com> --- mm/filemap.c | 5 +++++ 1 file changed, 5 insertions(+)