xfs
[Top] [All Lists]

Re: [PATCH] xfs_file_last_byte() needs to acquire ilock

To: Lachlan McIlroy <lmcilroy@xxxxxxxxxx>
Subject: Re: [PATCH] xfs_file_last_byte() needs to acquire ilock
From: Felix Blyakher <felixb@xxxxxxx>
Date: Thu, 23 Apr 2009 23:25:03 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1624785772.3251240539480564.JavaMail.root@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
References: <1624785772.3251240539480564.JavaMail.root@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>

On Apr 23, 2009, at 9:18 PM, Lachlan McIlroy wrote:

We had some systems crash with this stack:

[<a00000010000cb20>] ia64_leave_kernel+0x0/0x280
[<a00000021291ca00>] xfs_bmbt_get_startoff+0x0/0x20 [xfs]
[<a0000002129080b0>] xfs_bmap_last_offset+0x210/0x280 [xfs]
[<a00000021295b010>] xfs_file_last_byte+0x70/0x1a0 [xfs]
[<a00000021295b200>] xfs_itruncate_start+0xc0/0x1a0 [xfs]
[<a0000002129935f0>] xfs_inactive_free_eofblocks+0x290/0x460 [xfs]
[<a000000212998fb0>] xfs_release+0x1b0/0x240 [xfs]
[<a0000002129ad930>] xfs_file_release+0x70/0xa0 [xfs]
[<a000000100162ea0>] __fput+0x1a0/0x420
[<a000000100163160>] fput+0x40/0x60

The problem here is that xfs_file_last_byte() does not acquire the
inode lock and can therefore race with another thread that is modifying
the extext list.  While xfs_bmap_last_offset() is trying to lookup
what was the last extent some extents were merged and the extent list
shrunk so the index we lookup is now beyond the end of the extent list
and potentially in a freed buffer.

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e7ae08d..cf62d9d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1258,8 +1258,10 @@ xfs_file_last_byte(
         * necessary.
         */
        if (ip->i_df.if_flags & XFS_IFEXTENTS) {
+               xfs_ilock(ip, XFS_ILOCK_SHARED);
                error = xfs_bmap_last_offset(NULL, ip, &last_block,
                        XFS_DATA_FORK);
+               xfs_iunlock(ip, XFS_ILOCK_SHARED);
                if (error) {
                        last_block = 0;
                }

My understanding from the original patch was that this is one part
of the fix, and the other was the following change:

@@ -3206,6 +3208,8 @@ xfs_bmap_del_extent(
                 */
                XFS_BMAP_TRACE_DELETE("3", ip, idx, 1, whichfork);
                xfs_iext_remove(ifp, idx, 1);
+ if (idx >= (ifp->if_bytes / (uint)sizeof(xfs_bmbt_rec_t)))
+                       idx--;
                ifp->if_lastex = idx;
                if (delay)
                        break;

You don't think it's still needed, do you?

Felix

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