[RFC] racy use of ->d_name

[Mike Marshall <hubcap@xxxxxxxxxxxx>]

In a recent thread I read on fs-devel, Al Viro said:

AV> the use of ->d_name *is* racy, and while it might be
AV> tolerable for occasional debugging, for anything in heavier use it's
AV> asking for trouble.

Naturally <g> we use d_name in Orangefs some, so I looked at
some other code for inspiration on how to protect its usage.

I'm guessing this example is how I should protect simple usages
like this:

diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
index 5355efb..05321f4 100644
--- a/fs/orangefs/dcache.c
+++ b/fs/orangefs/dcache.c
@@ -30,9 +30,11 @@ static int orangefs_revalidate_lookup(struct dentry *dentry)

        new_op->upcall.req.lookup.sym_follow = ORANGEFS_LOOKUP_LINK_NO_FOLLOW;
        new_op->upcall.req.lookup.parent_refn = parent->refn;
+       spin_lock(&dentry->d_lock);
        strncpy(new_op->upcall.req.lookup.d_name,
                dentry->d_name.name,
                ORANGEFS_NAME_MAX);
+       spin_unlock(&dentry->d_lock);

        gossip_debug(GOSSIP_DCACHE_DEBUG,
                     "%s:%s:%d interrupt flag [%d]\n",


It seems that in debug statements, icky looking things like
file->f_path.dentry->d_name.name get replaced with "file" and "%pD".

I have a place where I use file->f_path.dentry->d_name.name in a
strcmp, and the way I "fixed" it seems verbose and hackerly... what
should I do instead?

diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c
index b5dbc9c..bb1e8a8 100644
--- a/fs/orangefs/orangefs-debugfs.c
+++ b/fs/orangefs/orangefs-debugfs.c
@@ -432,6 +432,8 @@ static ssize_t orangefs_debug_write(struct file *file,
        int rc = -EFAULT;
        size_t silly = 0;
        char *debug_string;
+       char *debug_file_name;
+       int debug_file_buf_len = strlen(ORANGEFS_KMOD_DEBUG_FILE) + 10;
        struct orangefs_kernel_op_s *new_op = NULL;
        struct client_debug_mask c_mask = { NULL, 0, 0 };

@@ -452,6 +454,11 @@ static ssize_t orangefs_debug_write(struct file *file,
        if (!buf)
                goto out;

+       debug_file_name =
+               kzalloc(debug_file_buf_len, GFP_KERNEL);
+       if (!debug_file_name)
+               goto out;
+
        if (copy_from_user(buf, ubuf, count - 1)) {
                gossip_debug(GOSSIP_DEBUGFS_DEBUG,
                             "%s: copy_from_user failed!\n",
@@ -468,8 +475,20 @@ static ssize_t orangefs_debug_write(struct file *file,
         * A service operation is required to set a new client-side
         * debug mask.
         */
-       if (!strcmp(file->f_path.dentry->d_name.name,
-                   ORANGEFS_KMOD_DEBUG_FILE)) {
+
+       /*
+        * We need to look and see whether they're setting the keyword
+        * string in the kernel debug file, or the client debug file.
+        * Fetching the filename out of "file" with "%pD" is better
+        * than fetching it out of file->f_path.dentry->d_name.name.
+        *
+        * debug_file_buf_len is longer than it needs to be in case
+        * some weirdo or malicious file that matches "the right thing
+        * and then some" gets passed into here somehow.
+        */
+       snprintf(debug_file_name, debug_file_buf_len, "%pD", file);
+
+       if (!strcmp(debug_file_name, ORANGEFS_KMOD_DEBUG_FILE)) {
                debug_string_to_mask(buf, &orangefs_gossip_debug_mask, 0);
                debug_mask_to_string(&orangefs_gossip_debug_mask, 0);
                debug_string = kernel_debug_string;
@@ -536,6 +555,7 @@ static ssize_t orangefs_debug_write(struct file *file,
                     "orangefs_debug_write: rc: %d\n",
                     rc);
        kfree(buf);
+       kfree(debug_file_name);
        return rc;
 }
--
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