Re: [PATCH] ext4: fix race between ext4_sync_parent() and rename()

On Wed, May 06, 2020 at 11:31:40AM -0700, Eric Biggers wrote:

From: Eric Biggers ebiggers@google.com

'igrab(d_inode(dentry->d_parent))' without holding dentry->d_lock is broken because without d_lock, d_parent can be concurrently changed due to a rename(). Then if the old directory is immediately deleted, old d_parent->inode can be NULL. That causes a NULL dereference in igrab().

To fix this, use dget_parent() to safely grab a reference to the parent dentry, which pins the inode. This also eliminates the need to use d_find_any_alias() other than for the initial inode, as we no longer throw away the dentry at each step.

This is an extremely hard race to hit, but it is possible. Adding a udelay() in between the reads of ->d_parent and its ->d_inode makes it reproducible on a no-journal filesystem using the following program:

#include <fcntl.h>
#include <unistd.h>

int main()
{
    if (fork()) {
        for (;;) {
            mkdir("dir1", 0700);
            int fd = open("dir1/file", O_RDWR|O_CREAT|O_SYNC);
            write(fd, "X", 1);
            close(fd);
        }
    } else {
        mkdir("dir2", 0700);
        for (;;) {
            rename("dir1/file", "dir2/file");
            rmdir("dir1");
        }
    }
}

Fixes: d59729f4e794 ("ext4: fix races in ext4_sync_parent()") Cc: stable@vger.kernel.org Signed-off-by: Eric Biggers ebiggers@google.com

Any comments on this patch?

- Eric