| Message ID | 5d7a3eabf63e5c60a8ceb243221bc8778117a8e8.1719867140.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix __folio_put with no deref in btrfs_do_encoded_write | expand |
在 2024/7/2 06:22, Boris Burkov 写道: > The conversion to folios switched __free_page to __folio_put in the > error path in btrfs_do_encoded_write. > > However, this gets the page refcounting wrong. If we do hit that error > path (I reproduced by modifying btrfs_do_encoded_write to pretend to > always fail in a way that jumps to out_folios and running the xfstest > btrfs/281), then we always hit the following BUG freeing the folio: > > BUG: Bad page state in process btrfs pfn:40ab0b > page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x61be5 pfn:0x40ab0b > flags: 0x5ffff0000000000(node=0|zone=2|lastcpupid=0x1ffff) > raw: 05ffff0000000000 0000000000000000 dead000000000122 0000000000000000 > raw: 0000000000061be5 0000000000000000 00000001ffffffff 0000000000000000 > page dumped because: nonzero _refcount > Call Trace: > <TASK> > dump_stack_lvl+0x3d/0xe0 > bad_page+0xea/0xf0 > free_unref_page+0x8e1/0x900 > ? __mem_cgroup_uncharge+0x69/0x90 > __folio_put+0xe6/0x190 > btrfs_do_encoded_write+0x445/0x780 > ? current_time+0x25/0xd0 > btrfs_do_write_iter+0x2cc/0x4b0 > btrfs_ioctl_encoded_write+0x2b6/0x340 > > It turns out __free_page dereferenced the page while __folio_put does > not. Switch __folio_put to folio_put which does dereference the page > first. > > Fixes: 400b172b8cdc ("btrfs: compression: migrate compression/decompression paths to folios") > Signed-off-by: Boris Burkov <boris@bur.io> My bad, I should go deeper before doing the switch from __free_page() to __folio_put(). Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks for catching this! Qu > --- > fs/btrfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 0a11d309ee89..12fb7e8056a1 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -9558,7 +9558,7 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from, > out_folios: > for (i = 0; i < nr_folios; i++) { > if (folios[i]) > - __folio_put(folios[i]); > + folio_put(folios[i]); > } > kvfree(folios); > out:
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 0a11d309ee89..12fb7e8056a1 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -9558,7 +9558,7 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from, out_folios: for (i = 0; i < nr_folios; i++) { if (folios[i]) - __folio_put(folios[i]); + folio_put(folios[i]); } kvfree(folios); out:
The conversion to folios switched __free_page to __folio_put in the error path in btrfs_do_encoded_write. However, this gets the page refcounting wrong. If we do hit that error path (I reproduced by modifying btrfs_do_encoded_write to pretend to always fail in a way that jumps to out_folios and running the xfstest btrfs/281), then we always hit the following BUG freeing the folio: BUG: Bad page state in process btrfs pfn:40ab0b page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x61be5 pfn:0x40ab0b flags: 0x5ffff0000000000(node=0|zone=2|lastcpupid=0x1ffff) raw: 05ffff0000000000 0000000000000000 dead000000000122 0000000000000000 raw: 0000000000061be5 0000000000000000 00000001ffffffff 0000000000000000 page dumped because: nonzero _refcount Call Trace: <TASK> dump_stack_lvl+0x3d/0xe0 bad_page+0xea/0xf0 free_unref_page+0x8e1/0x900 ? __mem_cgroup_uncharge+0x69/0x90 __folio_put+0xe6/0x190 btrfs_do_encoded_write+0x445/0x780 ? current_time+0x25/0xd0 btrfs_do_write_iter+0x2cc/0x4b0 btrfs_ioctl_encoded_write+0x2b6/0x340 It turns out __free_page dereferenced the page while __folio_put does not. Switch __folio_put to folio_put which does dereference the page first. Fixes: 400b172b8cdc ("btrfs: compression: migrate compression/decompression paths to folios") Signed-off-by: Boris Burkov <boris@bur.io> --- fs/btrfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)