| Message ID | 20221013081657.3508759-1-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: fix readdir cache race | expand |
On Thu, Oct 13, 2022 at 10:16:57AM +0200, Miklos Szeredi wrote: > Willy, > > delete_from_page_cache() exporting was just removed, and seems like it's > deprecated in favor of the folio functions. Should this code use the folio > one and export that? Is the code even correct to begin with? In general, all filesystem code should be converted from pages to folios. Whether you want to support multi-page folios is up to you; a little more complexity but a performance win. I'm not thrilled about exporting filemap_remove_folio() or delete_from_page_cache(). At the very least, please make it _GPL. What's the harm in leaving an extra page in the page cache, as long as it's initialised? > Thanks, > Miklos > > fs/fuse/readdir.c | 18 ++++++++++-------- > mm/folio-compat.c | 1 + > 2 files changed, 11 insertions(+), 8 deletions(-) > > diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c > index b4e565711045..4284e28be2e8 100644 > --- a/fs/fuse/readdir.c > +++ b/fs/fuse/readdir.c > @@ -65,25 +65,27 @@ static void fuse_add_dirent_to_cache(struct file *file, > page = find_lock_page(file->f_mapping, index); > } else { > page = find_or_create_page(file->f_mapping, index, > - mapping_gfp_mask(file->f_mapping)); > + mapping_gfp_mask(file->f_mapping) | __GFP_ZERO); > } > if (!page) > return; > > spin_lock(&fi->rdc.lock); > + addr = kmap_local_page(page); > /* Raced with another readdir */ > if (fi->rdc.version != version || fi->rdc.size != size || > - WARN_ON(fi->rdc.pos != pos)) > - goto unlock; > + WARN_ON(fi->rdc.pos != pos)) { > + /* Was this page just created? */ > + if (!offset && !((struct fuse_dirent *) addr)->namelen) > + delete_from_page_cache(page); > + goto unmap; > + } > > - addr = kmap_local_page(page); > - if (!offset) > - clear_page(addr); > memcpy(addr + offset, dirent, reclen); > - kunmap_local(addr); > fi->rdc.size = (index << PAGE_SHIFT) + offset + reclen; > fi->rdc.pos = dirent->off; > -unlock: > +unmap: > + kunmap_local(addr); > spin_unlock(&fi->rdc.lock); > unlock_page(page); > put_page(page); > diff --git a/mm/folio-compat.c b/mm/folio-compat.c > index e1e23b4947d7..83aaffa6d701 100644 > --- a/mm/folio-compat.c > +++ b/mm/folio-compat.c > @@ -128,6 +128,7 @@ void delete_from_page_cache(struct page *page) > { > return filemap_remove_folio(page_folio(page)); > } > +EXPORT_SYMBOL(delete_from_page_cache); > > int try_to_release_page(struct page *page, gfp_t gfp) > { > -- > 2.37.3 >
diff --git a/fs/fuse/readdir.c b/fs/fuse/readdir.c index b4e565711045..4284e28be2e8 100644 --- a/fs/fuse/readdir.c +++ b/fs/fuse/readdir.c @@ -65,25 +65,27 @@ static void fuse_add_dirent_to_cache(struct file *file, page = find_lock_page(file->f_mapping, index); } else { page = find_or_create_page(file->f_mapping, index, - mapping_gfp_mask(file->f_mapping)); + mapping_gfp_mask(file->f_mapping) | __GFP_ZERO); } if (!page) return; spin_lock(&fi->rdc.lock); + addr = kmap_local_page(page); /* Raced with another readdir */ if (fi->rdc.version != version || fi->rdc.size != size || - WARN_ON(fi->rdc.pos != pos)) - goto unlock; + WARN_ON(fi->rdc.pos != pos)) { + /* Was this page just created? */ + if (!offset && !((struct fuse_dirent *) addr)->namelen) + delete_from_page_cache(page); + goto unmap; + } - addr = kmap_local_page(page); - if (!offset) - clear_page(addr); memcpy(addr + offset, dirent, reclen); - kunmap_local(addr); fi->rdc.size = (index << PAGE_SHIFT) + offset + reclen; fi->rdc.pos = dirent->off; -unlock: +unmap: + kunmap_local(addr); spin_unlock(&fi->rdc.lock); unlock_page(page); put_page(page); diff --git a/mm/folio-compat.c b/mm/folio-compat.c index e1e23b4947d7..83aaffa6d701 100644 --- a/mm/folio-compat.c +++ b/mm/folio-compat.c @@ -128,6 +128,7 @@ void delete_from_page_cache(struct page *page) { return filemap_remove_folio(page_folio(page)); } +EXPORT_SYMBOL(delete_from_page_cache); int try_to_release_page(struct page *page, gfp_t gfp) {
There's a race in fuse's readdir cache that can result in an uninitilized page being read. The page lock is supposed to prevent this from happening but in the following case it doesn't: Two fuse_add_dirent_to_cache() start out and get the same parameters (size=0,offset=0). One of them wins the race to create and lock the page, after which it fills in data, sets rdc.size and unlocks the page. In the meantime the page gets evicted from the cache before the other instance gets to run. So that one also creates the page, but finds the size to be mismatched, bails out and leaves the uninitialized page in the cache. Fix by making sure that this spurious creation gets undone, so that the cache always contains valid data (while the page lock is held). The __GFP_ZERO is to differentiate this from the case when the page wasn't evicted between the racing additions, as well as a nice cleanup. Reported-by: Frank Sorenson <fsorenso@redhat.com> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- Willy, delete_from_page_cache() exporting was just removed, and seems like it's deprecated in favor of the folio functions. Should this code use the folio one and export that? Is the code even correct to begin with? Thanks, Miklos fs/fuse/readdir.c | 18 ++++++++++-------- mm/folio-compat.c | 1 + 2 files changed, 11 insertions(+), 8 deletions(-)