xfs
[Top] [All Lists]

Code Changes [Review Request]: xfs_trans_ail_delete() gets invoked with

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>, Dave Chinner <david@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Subject: Code Changes [Review Request]: xfs_trans_ail_delete() gets invoked with corrupted values – Causing Crash and Deadlock.
From: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
Date: Tue, 13 Sep 2011 17:37:25 +0530
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:date:message-id:subject:from:to:content-type :content-transfer-encoding; bh=+/SX+Yh/mfxI7IqLQv4iAG92HzFW7RBBkEkaxLOlfcE=; b=Ec2VCxjOHtPerUhRjXIAkn5EoATMv60GBCT3lBXrDccPtyTtFZDltyHFOQxGXPd9HN U3Gmhed95sVC/CZPC7sijJve2eAVE6vnKqvSU8lZLyeIPW5wM+eP73eSs9j69OZCIDPc RpD0WM5hi7bNb4L/2GiWPGIge627zKbxN9V8Q=
Architecture: ARM
Kernel: 2.6.38.13 (observed on 3.0.3 also)
Test Case: Unplug device during umount

BackTrace:
Unable to handle kernel NULL pointer dereference at virtual address 00000023
Process umount (pid: 220, stack limit = 0xe45f22e8)
Backtrace:
(xfs_log_move_tail+0x0/0x280) from [<c01ac3b0>]
(xfs_trans_ail_delete+0xbc/0x17c)
(xfs_trans_ail_delete+0x0/0x17c) from [<c01830c0>] (xfs_buf_iodone+0x64/0x7c)
(xfs_buf_iodone+0x0/0x7c) from [<c0183028>] (xfs_buf_do_callbacks+0x2c/0x3c)
(xfs_buf_do_callbacks+0x0/0x3c) from [<c0183260>]
(xfs_buf_iodone_callbacks+0x14c/0x174)
(xfs_buf_iodone_callbacks+0x0/0x174) from [<c01b660c>]
(xfs_buf_iodone_work+0x58/0x7c)
(xfs_buf_iodone_work+0x0/0x7c) from [<c01b66b4>] (xfs_buf_ioend+0x84/0x9c)
(xfs_buf_ioend+0x0/0x9c) from [<c01b6d60>] (xfs_bioerror+0x4c/0x54)
(xfs_bioerror+0x0/0x54) from [<c01b6dc4>] (xfs_bdstrat_cb+0x5c/0x6c)
(xfs_bdstrat_cb+0x0/0x6c) from [<c01b6858>] (xfs_flush_buftarg+0xe4/0x18c)
(xfs_flush_buftarg+0x0/0x18c) from [<c01b6920>] (xfs_free_buftarg+0x20/0x78)
(xfs_free_buftarg+0x0/0x78) from [<c01bdecc>] (xfs_close_devices+0x64/0x68)
(xfs_close_devices+0x0/0x68) from [<c01bdf44>] (xfs_fs_put_super+0x74/0x88)
(xfs_fs_put_super+0x0/0x88) from [<c00c3a9c>]
(generic_shutdown_super+0x7c/0x120)
(generic_shutdown_super+0x0/0x120) from [<c00c3b74>]
(kill_block_super+0x34/0x4c)
(kill_block_super+0x0/0x4c) from [<c00c2b54>]
(deactivate_locked_super+0x3c/0x5c)
(deactivate_locked_super+0x0/0x5c) from [<c00c2d60>]
(deactivate_super+0x5c/0x60)
(deactivate_super+0x0/0x60) from [<c00da79c>] (mntput_no_expire+0x9c/0xe8)
(mntput_no_expire+0x0/0xe8) from [<c00dae40>] (sys_umount+0x308/0x334)
(sys_umount+0x0/0x334) from [<c001ef80>] (ret_fast_syscall+0x0/0x30)
Code: e1a02007 e59f00bc e1a01006 eb055bd7 (e5d53023)
---[ end trace ddd9103bce1b5eae ]---
------------[ cut here ]------------

In the case of crash (back trace as shown), the log structure is
de-allocated as part of the xfs_log_unmount(), but there was a pending
callback which gets called after all these un-mount routine. Since,
log structure doesn’t hold valid values it causes OOPS when accessed.

In the function xfs_buf_iodone() : file xfs_buf_item.c

If Mount point and the log structure addresses are checked through
xfs_buf_t *bp, it shows correct values – valid address for mount point
and NULL for log(bp->b_mount,bp->b_mount->m_log). While if the same
are accessed through “ailp->xa_mount,ailp->xa_mount->m_log” – these
hold garbage values.

I have made few changes:
diff -Nurp linux-orig/fs/xfs/xfs_buf_item.c linux-Modified/fs/xfs/xfs_buf_item.c
--- linux-orig/fs/xfs/xfs_buf_item.c    2011-09-13 17:08:13.000000000 +0530
+++ linux-Modified/fs/xfs/xfs_buf_item.c        2011-09-13 17:05:43.000000000 
+0530
@@ -1065,7 +1065,9 @@ xfs_buf_iodone(
         *
         * Either way, AIL is useless if we're forcing a shutdown.
         */
-       spin_lock(&ailp->xa_lock);
-       xfs_trans_ail_delete(ailp, (xfs_log_item_t *)bip);
-       xfs_buf_item_free(bip);
+       if(bp->b_mount->m_log){
+               spin_lock(&ailp->xa_lock);
+               xfs_trans_ail_delete(ailp, (xfs_log_item_t *)bip);
+               xfs_buf_item_free(bip);
+       }
 }
diff -Nurp linux-orig/fs/xfs/xfs_trans_ail.c
linux-Modified/fs/xfs/xfs_trans_ail.c
--- linux-orig/fs/xfs/xfs_trans_ail.c   2011-06-02 12:10:52.000000000 +0530
+++ linux-Modified/fs/xfs/xfs_trans_ail.c       2011-09-13 17:06:31.000000000 
+0530
@@ -27,6 +27,7 @@
 #include "xfs_mount.h"
 #include "xfs_trans_priv.h"
 #include "xfs_error.h"
+#include "xfs_log_priv.h"

 STATIC void xfs_ail_insert(struct xfs_ail *, xfs_log_item_t *);
 STATIC xfs_log_item_t * xfs_ail_delete(struct xfs_ail *, xfs_log_item_t *);
@@ -619,6 +619,9 @@ xfs_trans_ail_destroy(
        struct xfs_ail  *ailp = mp->m_ail;

        xfsaild_stop(ailp);
+       mp->m_ail->xa_mount = NULL;
+        mp->m_ail = NULL;
+        mp->m_log->l_ailp = NULL;
        kmem_free(ailp);
 }

Please share your opinion on these changes.

Thanks & Regards,
Amit Sahrawat

<Prev in Thread] Current Thread [Next in Thread>