| Message ID | 20240120230824.2619716-2-willy@infradead.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ubifs folio conversion | expand |
在 2024/1/21 7:08, Matthew Wilcox (Oracle) 写道: > Page cache reads are lockless, so setting the freshly allocated page > uptodate before we've overwritten it with the data it's supposed to have > in it will allow a simultaneous reader to see old data. Move the call > to SetPageUptodate into ubifs_write_end(), which is after we copied the > new data into the page. > > Fixes: 1e51764a3c2a ("UBIFS: add new flash file system") > Cc: stable@vger.kernel.org > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > fs/ubifs/file.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c > index 5029eb3390a5..40a9b03ef821 100644 > --- a/fs/ubifs/file.c > +++ b/fs/ubifs/file.c > @@ -463,9 +463,6 @@ static int ubifs_write_begin(struct file *file, struct address_space *mapping, > return err; > } > } > - > - SetPageUptodate(page); > - ClearPageError(page); > } This solution looks good to me, and I think 'SetPageUptodate' should be removed from write_begin_slow(slow path) too. > > err = allocate_budget(c, page, ui, appending); > @@ -569,6 +566,9 @@ static int ubifs_write_end(struct file *file, struct address_space *mapping, > goto out; > } > > + if (len == PAGE_SIZE) > + SetPageUptodate(page); > + > if (!PagePrivate(page)) { > attach_page_private(page, (void *)1); > atomic_long_inc(&c->dirty_pg_cnt); >
On Mon, Jan 22, 2024 at 03:22:45PM +0800, Zhihao Cheng wrote: > 在 2024/1/21 7:08, Matthew Wilcox (Oracle) 写道: > > Page cache reads are lockless, so setting the freshly allocated page > > uptodate before we've overwritten it with the data it's supposed to have > > in it will allow a simultaneous reader to see old data. Move the call > > to SetPageUptodate into ubifs_write_end(), which is after we copied the > > new data into the page. > > This solution looks good to me, and I think 'SetPageUptodate' should be > removed from write_begin_slow(slow path) too. I didn't bother because we have just read into the page so it is uptodate. A racing read will see the data from before the write, but that's an acceptable ordering of events.
在 2024/1/22 22:40, Matthew Wilcox 写道: > On Mon, Jan 22, 2024 at 03:22:45PM +0800, Zhihao Cheng wrote: >> 在 2024/1/21 7:08, Matthew Wilcox (Oracle) 写道: >>> Page cache reads are lockless, so setting the freshly allocated page >>> uptodate before we've overwritten it with the data it's supposed to have >>> in it will allow a simultaneous reader to see old data. Move the call >>> to SetPageUptodate into ubifs_write_end(), which is after we copied the >>> new data into the page. >> >> This solution looks good to me, and I think 'SetPageUptodate' should be >> removed from write_begin_slow(slow path) too. > > I didn't bother because we have just read into the page so it is > uptodate. A racing read will see the data from before the write, but > that's an acceptable ordering of events. > . > I can't find where the page is read and set uptodate. I think the uninitialized data can be found in following path: writer reader ubifs_write_begin page1 = grab_cache_page_write_begin err = allocate_budget // ENOSPC unlock_page(page1) put_page(page1) write_begin_slow page2 = grab_cache_page_write_begin SetPageChecked(page2) SetPageUptodate(page2) generic_file_read_iter filemap_read filemap_get_pages filemap_get_read_batch if (!folio_test_uptodate) // page2 is uptodate copy_folio_to_iter // read uninitialized page content! copy_page_from_iter_atomic // copy data to cover uninitialized page content
On Tue, Jan 23, 2024 at 10:36:14AM +0800, Zhihao Cheng wrote: > 在 2024/1/22 22:40, Matthew Wilcox 写道: > > On Mon, Jan 22, 2024 at 03:22:45PM +0800, Zhihao Cheng wrote: > > > 在 2024/1/21 7:08, Matthew Wilcox (Oracle) 写道: > > > > Page cache reads are lockless, so setting the freshly allocated page > > > > uptodate before we've overwritten it with the data it's supposed to have > > > > in it will allow a simultaneous reader to see old data. Move the call > > > > to SetPageUptodate into ubifs_write_end(), which is after we copied the > > > > new data into the page. > > > > > > This solution looks good to me, and I think 'SetPageUptodate' should be > > > removed from write_begin_slow(slow path) too. > > > > I didn't bother because we have just read into the page so it is > > uptodate. A racing read will see the data from before the write, but > > that's an acceptable ordering of events. > > . > > > > I can't find where the page is read and set uptodate. I think the > uninitialized data can be found in following path: You're right; thanks. I'd misread the code. I'll send a new version in a few hours.
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c index 5029eb3390a5..40a9b03ef821 100644 --- a/fs/ubifs/file.c +++ b/fs/ubifs/file.c @@ -463,9 +463,6 @@ static int ubifs_write_begin(struct file *file, struct address_space *mapping, return err; } } - - SetPageUptodate(page); - ClearPageError(page); } err = allocate_budget(c, page, ui, appending); @@ -569,6 +566,9 @@ static int ubifs_write_end(struct file *file, struct address_space *mapping, goto out; } + if (len == PAGE_SIZE) + SetPageUptodate(page); + if (!PagePrivate(page)) { attach_page_private(page, (void *)1); atomic_long_inc(&c->dirty_pg_cnt);
Page cache reads are lockless, so setting the freshly allocated page uptodate before we've overwritten it with the data it's supposed to have in it will allow a simultaneous reader to see old data. Move the call to SetPageUptodate into ubifs_write_end(), which is after we copied the new data into the page. Fixes: 1e51764a3c2a ("UBIFS: add new flash file system") Cc: stable@vger.kernel.org Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- fs/ubifs/file.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)