[RFC] racy use of ->d_name

Commit Message

Mike Marshall Dec. 14, 2016, 7:35 p.m. UTC

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:

the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Al Viro Dec. 14, 2016, 9:08 p.m. UTC | #1

On Wed, Dec 14, 2016 at 02:35:39PM -0500, Mike Marshall wrote:

> 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",

That one is potentially nastier - ->d_revalidate() *can* race with
rename().  Next cycle we'll hopefully get to the point where this
kind of "we need to force lookup" will be handled outside of ->d_revalidate();
the main problem with doing so right now is that we would lose anything
mounted on that thing or anywhere under it and we will always get a new
dentry for non-directories.  AFAICS, your instance is safe with that ->d_lock,
but I would really prefer to have that done from ->lookup() rather than
duplicate between lookup and d_revalidate like now.  I have some bits and
pieces in that direction, but it was way too late in this cycle to be
4.10 material.

> 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?

debugfs has no rename, so there it's actually stable.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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