| Message ID | 20210727013942.83531-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: change the set_page_extent_mapped() call into an ASSERT() | expand |
On 2021/7/27 上午9:39, Qu Wenruo wrote: > Btrfs uses set_page_extent_mapped() to properly setup a page. > > That function would set PagePrivate, and populate needed structure for > subpage. > The timing of calling set_page_extent_mapped() happens before reading a > page or dirtying a page. > Thus when we got a page to write back, if it doesn't have PagePrivate, > it is a big problem in code logic. > > Calling set_page_extent_mapped() for such page would just mask the > problem. > Furthermore, for subpage case, we call subpage error helper to clear the > page error bit before calling set_page_extent_mapped(). > If we really got a page without Private bit, it can call kernel NULL > pointer dereference. > > So change the set_page_extent_mapped() call to an ASSERT(), and move the > check before any page status update call. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Please discard the patch. Although I haven't hit any problem testing the patch, it's still possible that we have a special page that would need cow fixup. Such page will be: - Dirty - Not Private Thus no page->private, this could still cause problem for subpage case though - No EXTENT_DELALLOC flags set for any range inside the page Thus writepage_delalloc() will not find a delalloc range inside the page. Such page will be caught by btrfs_writepage_cow_fixup(), but it will trigger the ASSERT() added by this patch. Thanks, Qu > --- > fs/btrfs/extent_io.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 62f0ed2de2b9..219add264acf 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -4099,6 +4099,12 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc, > > WARN_ON(!PageLocked(page)); > > + /* > + * All dirty page to be written should have PagePrivate set by > + * set_page_extent_mapped() when creating the page. > + */ > + ASSERT(PagePrivate(page)); > + > btrfs_page_clear_error(btrfs_sb(inode->i_sb), page, > page_offset(page), PAGE_SIZE); > > @@ -4115,12 +4121,6 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc, > flush_dcache_page(page); > } > > - ret = set_page_extent_mapped(page); > - if (ret < 0) { > - SetPageError(page); > - goto done; > - } > - > if (!epd->extent_locked) { > ret = writepage_delalloc(BTRFS_I(inode), page, wbc, > &nr_written); >
On Tue, Jul 27, 2021 at 06:29:08PM +0800, Qu Wenruo wrote: > > > On 2021/7/27 上午9:39, Qu Wenruo wrote: > > Btrfs uses set_page_extent_mapped() to properly setup a page. > > > > That function would set PagePrivate, and populate needed structure for > > subpage. > > The timing of calling set_page_extent_mapped() happens before reading a > > page or dirtying a page. > > Thus when we got a page to write back, if it doesn't have PagePrivate, > > it is a big problem in code logic. > > > > Calling set_page_extent_mapped() for such page would just mask the > > problem. > > Furthermore, for subpage case, we call subpage error helper to clear the > > page error bit before calling set_page_extent_mapped(). > > If we really got a page without Private bit, it can call kernel NULL > > pointer dereference. > > > > So change the set_page_extent_mapped() call to an ASSERT(), and move the > > check before any page status update call. > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > Please discard the patch. > > Although I haven't hit any problem testing the patch, it's still > possible that we have a special page that would need cow fixup. > > Such page will be: > > - Dirty > > - Not Private > Thus no page->private, this could still cause problem for subpage case > though > > - No EXTENT_DELALLOC flags set for any range inside the page > Thus writepage_delalloc() will not find a delalloc range inside the > page. > > Such page will be caught by btrfs_writepage_cow_fixup(), but it will > trigger the ASSERT() added by this patch. Hm yeah the mismatch between dirty/private/delalloc sounds exactly like work for the cow fixup.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 62f0ed2de2b9..219add264acf 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -4099,6 +4099,12 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc, WARN_ON(!PageLocked(page)); + /* + * All dirty page to be written should have PagePrivate set by + * set_page_extent_mapped() when creating the page. + */ + ASSERT(PagePrivate(page)); + btrfs_page_clear_error(btrfs_sb(inode->i_sb), page, page_offset(page), PAGE_SIZE); @@ -4115,12 +4121,6 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc, flush_dcache_page(page); } - ret = set_page_extent_mapped(page); - if (ret < 0) { - SetPageError(page); - goto done; - } - if (!epd->extent_locked) { ret = writepage_delalloc(BTRFS_I(inode), page, wbc, &nr_written);
Btrfs uses set_page_extent_mapped() to properly setup a page. That function would set PagePrivate, and populate needed structure for subpage. The timing of calling set_page_extent_mapped() happens before reading a page or dirtying a page. Thus when we got a page to write back, if it doesn't have PagePrivate, it is a big problem in code logic. Calling set_page_extent_mapped() for such page would just mask the problem. Furthermore, for subpage case, we call subpage error helper to clear the page error bit before calling set_page_extent_mapped(). If we really got a page without Private bit, it can call kernel NULL pointer dereference. So change the set_page_extent_mapped() call to an ASSERT(), and move the check before any page status update call. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)