All...
WRT: fs/orangefs/symlink.c...
AV> Said that, there is an unpleasant bug in that area - link_target of a live
AV> inode can be overwritten, right under the pathname resolution walking the
AV> old contents of that thing.
AV> copy_attributes_to_inode() is triggered by ->d_revalidate() and
AV> by ->getattr() and it's really, really unsafe for a live inode.
I can see how that would be unsafe, but have had to look at a lot
of other people's code, and in the Documentation directory, and
think a lot (ouch it hurts when I do that) to see how to try and make
it safe.
./filesystems/vfs.txt
d_revalidate: called when the VFS needs to revalidate a dentry. This
is called whenever a name look-up finds a dentry in the
dcache. Most local filesystems leave this as NULL, because all their
dentries in the dcache are valid. Network filesystems are different
since things can change on the server without the client necessarily
being aware of it.
This function should return a positive value if the dentry is still
valid, and zero or a negative error code if it isn't.
Perhaps d_revalidate didn't have any business triggering
copy_attributes_to_inode? I've made this change, and have no
regressions with dbench or other silly tests, I'm about to leave the
office for the afternoon with xfstests running.
Do you think this change fixes the bug?
$ git diff
diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
index 5dd9841..0419981 100644
--- a/fs/orangefs/dcache.c
+++ b/fs/orangefs/dcache.c
@@ -77,7 +77,7 @@ out_drop:
/*
* Verify that dentry is valid.
*
- * Should return 1 if dentry can still be trusted, else 0
+ * Should return 1 if dentry can still be trusted, else 0.
*/
static int orangefs_d_revalidate(struct dentry *dentry, unsigned int flags)
{
@@ -92,49 +92,27 @@ static int orangefs_d_revalidate(struct dentry *dentry, unsi
/* find inode from dentry */
if (!dentry->d_inode) {
- gossip_debug(GOSSIP_DCACHE_DEBUG, "%s: negative dentry.\n",
+ gossip_debug(GOSSIP_DCACHE_DEBUG,
+ "%s: negative dentry.\n",
__func__);
- goto invalid_exit;
+ goto out;
}
gossip_debug(GOSSIP_DCACHE_DEBUG, "%s: inode valid.\n", __func__);
inode = dentry->d_inode;
- /*
- * first perform a lookup to make sure that the object not only
- * exists, but is still in the expected place in the name space
- */
- if (!is_root_handle(inode)) {
- if (!orangefs_revalidate_lookup(dentry))
- goto invalid_exit;
- } else {
- gossip_debug(GOSSIP_DCACHE_DEBUG,
- "%s: root handle, lookup skipped.\n",
- __func__);
+ /* skip root handle lookups. */
+ if (is_root_handle(inode)) {
+ ret = 1;
+ goto out;
}
- /* now perform getattr */
- gossip_debug(GOSSIP_DCACHE_DEBUG,
- "%s: doing getattr: inode: %p, handle: %pU\n",
- __func__,
- inode,
- get_khandle_from_ino(inode));
- ret = orangefs_inode_getattr(inode, ORANGEFS_ATTR_SYS_ALL_NOHINT);
- gossip_debug(GOSSIP_DCACHE_DEBUG,
- "%s: getattr %s (ret = %d), returning %s for dentry i_count
- __func__,
- (ret == 0 ? "succeeded" : "failed"),
- ret,
- (ret == 0 ? "valid" : "INVALID"),
- atomic_read(&inode->i_count));
- if (ret != 0)
- goto invalid_exit;
-
- /* dentry is valid! */
- return 1;
-
-invalid_exit:
- return 0;
+ /* lookup the object. */
+ if (orangefs_revalidate_lookup(dentry))
+ ret = 1;
+
+out:
+ return ret;
}
const struct dentry_operations orangefs_dentry_operations = {
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
