Message-ID: <862b6c718957aff7156bf04964b7242f5075e8a7.camel@nokia.com>
Date:   Wed, 11 Mar 2020 17:06:16 +0000
From:   "Rantala, Tommi T. (Nokia - FI/Espoo)" <tommi.t.rantala@...ia.com>
To:     "hch@....de" <hch@....de>,
        "david@...morbit.com" <david@...morbit.com>
CC:     "darrick.wong@...cle.com" <darrick.wong@...cle.com>,
        "linux-xfs@...r.kernel.org" <linux-xfs@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: 5.5 XFS getdents regression?

On Wed, 2020-03-11 at 09:14 +1100, Dave Chinner wrote:
> On Tue, Mar 10, 2020 at 08:45:58AM +0000, Rantala, Tommi T. (Nokia -
> FI/Espoo) wrote:
> > Hello,
> > 
> > One of my GitLab CI jobs stopped working after upgrading server
> > 5.4.18-
> > 100.fc30.x86_64 -> 5.5.7-100.fc30.x86_64.
> > (tested 5.5.8-100.fc30.x86_64 too, no change)
> > The server is fedora30 with XFS rootfs.
> > The problem reproduces always, and takes only couple minutes to run.
> > 
> > The CI job fails in the beginning when doing "git clean" in docker
> > container, and failing to rmdir some directory:
> > "warning: failed to remove 
> > .vendor/pkg/mod/golang.org/x/net@...0.0-20200114155413-6afb5195e5aa/in
> > tern
> > al/socket: Directory not empty"
> > 
> > Quick google search finds some other people reporting similar problems
> > with 5.5.0:
> > https://gitlab.com/gitlab-org/gitlab-runner/issues/3185
> 
> Which appears to be caused by multiple gitlab processes modifying
> the directory at the same time. i.e. something is adding an entry to
> the directory at the same time something is trying to rm -rf it.
> That's a race condition, and would lead to the exact symptoms you
> see here, depending on where in the directory the new entry is
> added.

OK traced "execve" with strace too, and it shows that it's "git clean
-ffdx" command (single process) that is being executed in the container,
which is doing the cleanup.

Tested with 5.6-rc5, it's failing the same way.

Spent some time to bisect this, and the problem is introduced by this:

commit 263dde869bd09b1a709fd92118c7fff832773689
Author: Christoph Hellwig <hch@....de>
Date:   Fri Nov 8 15:05:32 2019 -0800

    xfs: cleanup xfs_dir2_block_getdents
    
    Use an offset as the main means for iteration, and only do pointer
    arithmetics to find the data/unused entries.
    
    Signed-off-by: Christoph Hellwig <hch@....de>
    Reviewed-by: Darrick J. Wong <darrick.wong@...cle.com>
    Signed-off-by: Darrick J. Wong <darrick.wong@...cle.com>



Hmmmmm, looking at that commit, I think it slighty changed how the
"offset" is used compared to how the pointers were used.

This cures the issue for me, tested (briefly) on top of 5.6-rc5.
Does it make sense...?
(Email client probably damages white-space, sorry, I'll send this properly
signed-off with git-send-email if it's OK)


diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
index 0d3b640cf1cc..af945ec9df3b 100644
--- a/fs/xfs/xfs_dir2_readdir.c
+++ b/fs/xfs/xfs_dir2_readdir.c
@@ -179,6 +179,7 @@ xfs_dir2_block_getdents(
        struct xfs_dir2_data_unused     *dup = bp->b_addr + offset;
        struct xfs_dir2_data_entry      *dep = bp->b_addr + offset;
        uint8_t filetype;
+       unsigned int dep_offset;
 
        /*
         * Unused, skip it.
@@ -188,18 +189,21 @@ xfs_dir2_block_getdents(
                continue;
        }
 
+       dep_offset = offset;
+
        /*
-        * Bump pointer for the next iteration.
+        * Bump offset for the next iteration.
         */
        offset += xfs_dir2_data_entsize(dp->i_mount, dep->namelen);

        /*
         * The entry is before the desired starting point, skip it.
         */
-       if (offset < wantoff)
+       if (dep_offset < wantoff)
                continue;
 
-       cook = xfs_dir2_db_off_to_dataptr(geo, geo->datablk, offset);
+       cook = xfs_dir2_db_off_to_dataptr(geo, geo->datablk,
+                                         dep_offset);
 
        ctx->pos = cook & 0x7fffffff;
        filetype = xfs_dir2_data_get_ftype(dp->i_mount, dep);



> > Collected some data with strace, and it seems that getdents is not
> > returning all entries:
> > 
> > 5.4 getdents64() returns 52+50+1+0 entries 
> > => all files in directory are deleted and rmdir() is OK
> > 
> > 5.5 getdents64() returns 52+50+0+0 entries
> > => rmdir() fails with ENOTEMPTY
> 
> Yup, that's a classic userspace TOCTOU race.
> 
> Remember, getdents() is effectively a sequential walk through the
> directory data - subsequent calls start at the offset (cookie) where
> the previous one left off. New entries can be added between
> getdents() syscalls.
> 
> If that new entry is put at the tail of the directory, then the last
> getdents() call will return that entry rather than none because it
> was placed at an offset in the directory that the getdents() sweep
> has not yet reached, and hence will be found by a future getdents()
> call in the sweep.
> 
> 
> However, if there is a hole in the directory structure before the
> current getdents cookie offset, a new entry can be added in that
> hole. i.e. at an offset in the directory that getdents has already
> passed over. That dirent will never be reported by the current
> getdents() sequence - a directory rewind and re-read is required to
> find it. i.e. there's an inherent userspace TOUTOC race condition in
> 'rm -rf' operations.
> 
> IOWs, this is exactly what you'd expect to see when there are
> concurrent userspace modifications to a directory that is currently
> being read. Hence you need to rule out an application and userspace
> level issues before looking for filesystem level problems.
> 
> Cheers,
> 
> Dave.

Powered by blists - more mailing lists