On Tue, May 07, 2013 at 05:45:20PM -0500, Mark Tinguely wrote:
> On 05/07/13 17:22, Dave Chinner wrote:
> >On Tue, May 07, 2013 at 03:24:28PM -0500, Mark Tinguely wrote:
> >>On 05/07/13 15:22, Dave Jones wrote:
> >>>On Tue, May 07, 2013 at 03:04:33PM -0500, Mark Tinguely wrote:
> >>> > On 05/07/13 14:59, Dave Jones wrote:
> >>> > > On Tue, May 07, 2013 at 02:58:15PM -0500, Mark Tinguely wrote:
> >>> > >
> >>> > > > > I can hit this almost instantly with fsx. I'll do a bisect, though
> >>> > > > > it sounds like you already have a suspect.
> >>> > > > >
> >>> > > >
> >>> > > > If you want to try kmem debug of Linux 3.8 that would help.
> >>> > >
> >>> > > I'm not sure what that is.
> >>> >
> >>> > Sorry, if you would test Linux 3.8 with "CONFIG_DEBUG_SLAB=y".
> >>>
> >>>Ah, done that. (I pretty much always run with it).
> >>>
> >>>This is something new. Even 3.9 was fine. It's only since
> >>>the recent xfs merge.
> >>>
> >>> Dave
> >>>
> >>
> >>git revert 666d644cd72a9ec58b353209ff191d7430f3b357
> >
> >That won't prevent the use after free. That commit fixed a problem
> >that could lead to a use after free, but what we are seeing here is
> >that it has ultimately exposed a previously unknown issue that
> >causes the use after free.
> >
> >Basically what is happening is that there are two commits for the
> >EFD being processed, when there should only be one. I'm not sure how
> >this is happening yet, but these three traces came out from my debug
> >sequentially when running generic/006:
>
> Sorry for the misleading statement. Yes, I agree that patch is a
> good thing. I meant that Dave and only Dave revert it and only to
> test if that patch was the change that caused the new symptom -
> which we know now that it is.
Sure, I realise that, and it turns out I'm wrong - it is a bug in
commit 666d644cd. Poisoning turns a "will probably never occur"
problem into an instant reproducer, because it sets a bit in the efi
structure that is normally zero when the EFI is freed and hence
triggers a second free of the EFI when reading it after the first
free....
Dave, the patch below should fix the problem.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
xfs: Don't reference the EFI after it is freed
From: Dave Chinner <dchinner@xxxxxxxxxx>
Checking the EFI for whether it is being released from recovery
after we've already released the known active reference is a mistake
worthy of a brown paper bag. Fix the (now) obvious use after free
that it can cause.
Reported-by: Dave Jones <davej@xxxxxxxxxx>
Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
---
fs/xfs/xfs_extfree_item.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
index c0f3750..98c437d 100644
--- a/fs/xfs/xfs_extfree_item.c
+++ b/fs/xfs/xfs_extfree_item.c
@@ -305,10 +305,22 @@ xfs_efi_release(xfs_efi_log_item_t *efip,
{
ASSERT(atomic_read(&efip->efi_next_extent) >= nextents);
if (atomic_sub_and_test(nextents, &efip->efi_next_extent)) {
+ int recovered;
+
+ /*
+ * __xfs_efi_release() can release the last reference to the EFI
+ * and free it, so it is unsafe to reference it after we've
+ * released the reference. The only case this is safe to do is
+ * if we are in recovery and the XFS_EFI_RECOVERED bit is set,
+ * meaning that we have two references to release. Check the
+ * recovered bit before the initial release, as we cannot
+ * reliably check it afterwards.
+ */
+ recovered = test_bit(XFS_EFI_RECOVERED, &efip->efi_flags);
__xfs_efi_release(efip);
/* recovery needs us to drop the EFI reference, too */
- if (test_bit(XFS_EFI_RECOVERED, &efip->efi_flags))
+ if (recovered)
__xfs_efi_release(efip);
}
}
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
