Skip to content

Commit

Permalink
afs: Fix incorrect triggering of sillyrename on 3rd-party invalidation
Browse files Browse the repository at this point in the history
[ Upstream commit 63d49d8 ]

The AFS filesystem is currently triggering the silly-rename cleanup from
afs_d_revalidate() when it sees that a dentry has been changed by a third
party[1].  It should not be doing this as the cleanup includes deleting the
silly-rename target file on iput.

Fix this by removing the places in the d_revalidate handling that validate
anything other than the directory and the dirent.  It probably should not
be looking to validate the target inode of the dentry also.

This includes removing the point in afs_d_revalidate() where the inode that
a dentry used to point to was marked as being deleted (AFS_VNODE_DELETED).
We don't know it got deleted.  It could have been renamed or it could have
hard links remaining.

This was reproduced by cloning a git repo onto an afs volume on one
machine, switching to another machine and doing "git status", then
switching back to the first and doing "git status".  The second status
would show weird output due to ".git/index" getting deleted by the above
mentioned mechanism.

A simpler way to do it is to do:

	machine 1: touch a
	machine 2: touch b; mv -f b a
	machine 1: stat a

on an afs volume.  The bug shows up as the stat failing with ENOENT and the
file server log showing that machine 1 deleted "a".

Fixes: 79ddbfa ("afs: Implement sillyrename for unlink and rename")
Reported-by: Markus Suvanto <markus.suvanto@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Markus Suvanto <markus.suvanto@gmail.com>
cc: linux-afs@lists.infradead.org
Link: https://bugzilla.kernel.org/show_bug.cgi?id=214217#c4 [1]
Link: https://lore.kernel.org/r/163111668100.283156.3851669884664475428.stgit@warthog.procyon.org.uk/
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
dhowells authored and gregkh committed Sep 30, 2021
1 parent d130b5f commit 95671c6
Showing 1 changed file with 7 additions and 39 deletions.
46 changes: 7 additions & 39 deletions fs/afs/dir.c
Expand Up @@ -1077,9 +1077,9 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry,
*/
static int afs_d_revalidate_rcu(struct dentry *dentry)
{
struct afs_vnode *dvnode, *vnode;
struct afs_vnode *dvnode;
struct dentry *parent;
struct inode *dir, *inode;
struct inode *dir;
long dir_version, de_version;

_enter("%p", dentry);
Expand Down Expand Up @@ -1109,18 +1109,6 @@ static int afs_d_revalidate_rcu(struct dentry *dentry)
return -ECHILD;
}

/* Check to see if the vnode referred to by the dentry still
* has a callback.
*/
if (d_really_is_positive(dentry)) {
inode = d_inode_rcu(dentry);
if (inode) {
vnode = AFS_FS_I(inode);
if (!afs_check_validity(vnode))
return -ECHILD;
}
}

return 1; /* Still valid */
}

Expand Down Expand Up @@ -1156,17 +1144,7 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
if (IS_ERR(key))
key = NULL;

if (d_really_is_positive(dentry)) {
inode = d_inode(dentry);
if (inode) {
vnode = AFS_FS_I(inode);
afs_validate(vnode, key);
if (test_bit(AFS_VNODE_DELETED, &vnode->flags))
goto out_bad;
}
}

/* lock down the parent dentry so we can peer at it */
/* Hold the parent dentry so we can peer at it */
parent = dget_parent(dentry);
dir = AFS_FS_I(d_inode(parent));

Expand All @@ -1175,7 +1153,7 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)

if (test_bit(AFS_VNODE_DELETED, &dir->flags)) {
_debug("%pd: parent dir deleted", dentry);
goto out_bad_parent;
goto not_found;
}

/* We only need to invalidate a dentry if the server's copy changed
Expand All @@ -1201,12 +1179,12 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
case 0:
/* the filename maps to something */
if (d_really_is_negative(dentry))
goto out_bad_parent;
goto not_found;
inode = d_inode(dentry);
if (is_bad_inode(inode)) {
printk("kAFS: afs_d_revalidate: %pd2 has bad inode\n",
dentry);
goto out_bad_parent;
goto not_found;
}

vnode = AFS_FS_I(inode);
Expand All @@ -1228,9 +1206,6 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
dentry, fid.unique,
vnode->fid.unique,
vnode->vfs_inode.i_generation);
write_seqlock(&vnode->cb_lock);
set_bit(AFS_VNODE_DELETED, &vnode->flags);
write_sequnlock(&vnode->cb_lock);
goto not_found;
}
goto out_valid;
Expand All @@ -1245,7 +1220,7 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
default:
_debug("failed to iterate dir %pd: %d",
parent, ret);
goto out_bad_parent;
goto not_found;
}

out_valid:
Expand All @@ -1256,16 +1231,9 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
_leave(" = 1 [valid]");
return 1;

/* the dirent, if it exists, now points to a different vnode */
not_found:
spin_lock(&dentry->d_lock);
dentry->d_flags |= DCACHE_NFSFS_RENAMED;
spin_unlock(&dentry->d_lock);

out_bad_parent:
_debug("dropping dentry %pd2", dentry);
dput(parent);
out_bad:
key_put(key);

_leave(" = 0 [bad]");
Expand Down

0 comments on commit 95671c6

Please sign in to comment.