* [PATCH] xfs: bufferhead chains are invalid after end_page_writeback
@ 2016-07-20 0:21 Dave Chinner
2016-07-21 14:16 ` Brian Foster
2016-07-21 14:32 ` Christoph Hellwig
0 siblings, 2 replies; 4+ messages in thread
From: Dave Chinner @ 2016-07-20 0:21 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
In xfs_finish_page_writeback(), we have a loop that looks like this:
do {
if (off < bvec->bv_offset)
goto next_bh;
if (off > end)
break;
bh->b_end_io(bh, !error);
next_bh:
off += bh->b_size;
} while ((bh = bh->b_this_page) != head);
The b_end_io function is end_buffer_async_write(), which will call
end_page_writeback() once all the buffers have marked as no longer
under IO. This issue here is that the only thing currently
protecting both the bufferhead chain and the page from being
reclaimed is the PageWriteback state held on the page.
While we attempt to limit the loop to just the buffers covered by
the IO, we still read from the buffer size and follow the next
pointer in the bufferhead chain. There is no guarantee that either
of these are valid after the PageWriteback flag has been cleared.
Hence, loops like this are completely unsafe, and result in
use-after-free issues. One such problem was caught by Calvin Owens
with KASAN:
.....
INFO: Freed in 0x103fc80ec age=18446651500051355200 cpu=2165122683 pid=-1
free_buffer_head+0x41/0x90
__slab_free+0x1ed/0x340
kmem_cache_free+0x270/0x300
free_buffer_head+0x41/0x90
try_to_free_buffers+0x171/0x240
xfs_vm_releasepage+0xcb/0x3b0
try_to_release_page+0x106/0x190
shrink_page_list+0x118e/0x1a10
shrink_inactive_list+0x42c/0xdf0
shrink_zone_memcg+0xa09/0xfa0
shrink_zone+0x2c3/0xbc0
.....
Call Trace:
<IRQ> [<ffffffff81e8b8e4>] dump_stack+0x68/0x94
[<ffffffff8153a995>] print_trailer+0x115/0x1a0
[<ffffffff81541174>] object_err+0x34/0x40
[<ffffffff815436e7>] kasan_report_error+0x217/0x530
[<ffffffff81543b33>] __asan_report_load8_noabort+0x43/0x50
[<ffffffff819d651f>] xfs_destroy_ioend+0x3bf/0x4c0
[<ffffffff819d69d4>] xfs_end_bio+0x154/0x220
[<ffffffff81de0c58>] bio_endio+0x158/0x1b0
[<ffffffff81dff61b>] blk_update_request+0x18b/0xb80
[<ffffffff821baf57>] scsi_end_request+0x97/0x5a0
[<ffffffff821c5558>] scsi_io_completion+0x438/0x1690
[<ffffffff821a8d95>] scsi_finish_command+0x375/0x4e0
[<ffffffff821c3940>] scsi_softirq_done+0x280/0x340
Where the access is occuring during IO completion after the buffer
had been freed from direct memory reclaim.
Prevent use-after-free accidents in this end_io processing loop by
pre-calculating the loop conditionals before calling bh->b_end_io().
The loop is already limited to just the bufferheads covered by the
IO in progress, so the offset checks are sufficient to prevent
accessing buffers in the chain after end_page_writeback() has been
called by the the bh->b_end_io() callout.
Yet another example of why Bufferheads Must Die.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reported-and-Tested-by: Calvin Owens <calvinowens@fb.com>
---
fs/xfs/xfs_aops.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 80714eb..0cfb944 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -87,6 +87,12 @@ xfs_find_bdev_for_inode(
* We're now finished for good with this page. Update the page state via the
* associated buffer_heads, paying attention to the start and end offsets that
* we need to process on the page.
+ *
+ * Landmine Warning: bh->b_end_io() will call end_page_writeback() on the last
+ * buffer in the IO. Once it does this, it is unsafe to access the bufferhead or
+ * the page at all, as we may be racing with memory reclaim and it can free both
+ * the bufferhead chain and the page as it will see the page as clean and
+ * unused.
*/
static void
xfs_finish_page_writeback(
@@ -95,8 +101,9 @@ xfs_finish_page_writeback(
int error)
{
unsigned int end = bvec->bv_offset + bvec->bv_len - 1;
- struct buffer_head *head, *bh;
+ struct buffer_head *head, *bh, *next;
unsigned int off = 0;
+ unsigned int bsize;
ASSERT(bvec->bv_offset < PAGE_SIZE);
ASSERT((bvec->bv_offset & ((1 << inode->i_blkbits) - 1)) == 0);
@@ -105,15 +112,17 @@ xfs_finish_page_writeback(
bh = head = page_buffers(bvec->bv_page);
+ bsize = bh->b_size;
do {
+ next = bh->b_this_page;
if (off < bvec->bv_offset)
goto next_bh;
if (off > end)
break;
bh->b_end_io(bh, !error);
next_bh:
- off += bh->b_size;
- } while ((bh = bh->b_this_page) != head);
+ off += bsize;
+ } while ((bh = next) != head);
}
/*
--
2.8.0.rc3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: bufferhead chains are invalid after end_page_writeback
2016-07-20 0:21 [PATCH] xfs: bufferhead chains are invalid after end_page_writeback Dave Chinner
@ 2016-07-21 14:16 ` Brian Foster
2016-07-21 14:32 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: Brian Foster @ 2016-07-21 14:16 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Wed, Jul 20, 2016 at 10:21:23AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> In xfs_finish_page_writeback(), we have a loop that looks like this:
>
> do {
> if (off < bvec->bv_offset)
> goto next_bh;
> if (off > end)
> break;
> bh->b_end_io(bh, !error);
> next_bh:
> off += bh->b_size;
> } while ((bh = bh->b_this_page) != head);
>
> The b_end_io function is end_buffer_async_write(), which will call
> end_page_writeback() once all the buffers have marked as no longer
> under IO. This issue here is that the only thing currently
> protecting both the bufferhead chain and the page from being
> reclaimed is the PageWriteback state held on the page.
>
> While we attempt to limit the loop to just the buffers covered by
> the IO, we still read from the buffer size and follow the next
> pointer in the bufferhead chain. There is no guarantee that either
> of these are valid after the PageWriteback flag has been cleared.
> Hence, loops like this are completely unsafe, and result in
> use-after-free issues. One such problem was caught by Calvin Owens
> with KASAN:
>
...
>
>
> Where the access is occuring during IO completion after the buffer
> had been freed from direct memory reclaim.
>
> Prevent use-after-free accidents in this end_io processing loop by
> pre-calculating the loop conditionals before calling bh->b_end_io().
> The loop is already limited to just the bufferheads covered by the
> IO in progress, so the offset checks are sufficient to prevent
> accessing buffers in the chain after end_page_writeback() has been
> called by the the bh->b_end_io() callout.
>
> Yet another example of why Bufferheads Must Die.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reported-and-Tested-by: Calvin Owens <calvinowens@fb.com>
> ---
Reviewed-by: Brian Foster <bfoster@redhat.com>
> fs/xfs/xfs_aops.c | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 80714eb..0cfb944 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -87,6 +87,12 @@ xfs_find_bdev_for_inode(
> * We're now finished for good with this page. Update the page state via the
> * associated buffer_heads, paying attention to the start and end offsets that
> * we need to process on the page.
> + *
> + * Landmine Warning: bh->b_end_io() will call end_page_writeback() on the last
> + * buffer in the IO. Once it does this, it is unsafe to access the bufferhead or
> + * the page at all, as we may be racing with memory reclaim and it can free both
> + * the bufferhead chain and the page as it will see the page as clean and
> + * unused.
> */
> static void
> xfs_finish_page_writeback(
> @@ -95,8 +101,9 @@ xfs_finish_page_writeback(
> int error)
> {
> unsigned int end = bvec->bv_offset + bvec->bv_len - 1;
> - struct buffer_head *head, *bh;
> + struct buffer_head *head, *bh, *next;
> unsigned int off = 0;
> + unsigned int bsize;
>
> ASSERT(bvec->bv_offset < PAGE_SIZE);
> ASSERT((bvec->bv_offset & ((1 << inode->i_blkbits) - 1)) == 0);
> @@ -105,15 +112,17 @@ xfs_finish_page_writeback(
>
> bh = head = page_buffers(bvec->bv_page);
>
> + bsize = bh->b_size;
> do {
> + next = bh->b_this_page;
> if (off < bvec->bv_offset)
> goto next_bh;
> if (off > end)
> break;
> bh->b_end_io(bh, !error);
> next_bh:
> - off += bh->b_size;
> - } while ((bh = bh->b_this_page) != head);
> + off += bsize;
> + } while ((bh = next) != head);
> }
>
> /*
> --
> 2.8.0.rc3
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: bufferhead chains are invalid after end_page_writeback
2016-07-20 0:21 [PATCH] xfs: bufferhead chains are invalid after end_page_writeback Dave Chinner
2016-07-21 14:16 ` Brian Foster
@ 2016-07-21 14:32 ` Christoph Hellwig
2016-07-21 23:15 ` Dave Chinner
1 sibling, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2016-07-21 14:32 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
(should probably go into 4.7 still..)
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xfs: bufferhead chains are invalid after end_page_writeback
2016-07-21 14:32 ` Christoph Hellwig
@ 2016-07-21 23:15 ` Dave Chinner
0 siblings, 0 replies; 4+ messages in thread
From: Dave Chinner @ 2016-07-21 23:15 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Thu, Jul 21, 2016 at 07:32:21AM -0700, Christoph Hellwig wrote:
> Looks fine,
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> (should probably go into 4.7 still..)
I'll tag it for -stable so it gets back there appropriately. The
problem exists in kernels long before the changes we've made
recently, however, but I don't think this fix will apply cleanly to
older kernels...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-07-21 23:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-20 0:21 [PATCH] xfs: bufferhead chains are invalid after end_page_writeback Dave Chinner
2016-07-21 14:16 ` Brian Foster
2016-07-21 14:32 ` Christoph Hellwig
2016-07-21 23:15 ` Dave Chinner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.