linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Possible FS race condition between vfs_rename and do_linkat (fs/namei.c)
@ 2019-08-24  7:24 Xavier Roche
  2019-08-24 10:01 ` Xavier Roche
  0 siblings, 1 reply; 3+ messages in thread
From: Xavier Roche @ 2019-08-24  7:24 UTC (permalink / raw)
  To: linux-fsdevel

Dear distinguished filesystem contributors,

There seem to be a race condition between vfs_rename and do_linkat,
when those operations are done in parallel:

1. Moving a file to a target file (eg. mv file target)
2. Creating a link from the target file (eg. ln target link)

My understanding is that as the target file is never erased on client
side, but just replaced, the link should never fail.

But maybe this is something the filesystem can not guarantee at all
(w.r.t POSIX typically) ?

To demonstrate this issue, just run the following script (it will in
loop move "file" to "target", and in parallel link "target" to "link")
:

========== Cut here ==========
#!/bin/bash
#

rm -f link file target
touch target

# Link target -> link in loop
while ln target link && rm link; do :; done &

# Overwrite file -> target in loop
while touch file && mv file target; do :; done &

wait
========== Cut here ==========

Running the script will yield:
./bug.sh
ln: failed to create hard link 'link' => 'target': No such file or directory

The issue seem to lie inside vfs_link (fs/namei.c):
       inode_lock(inode);
       /* Make sure we don't allow creating hardlink to an unlinked file */
       if (inode->i_nlink == 0 && !(inode->i_state & I_LINKABLE))
               error =  -ENOENT;

The possible answer is that the inode refcount is zero because the
file has just been replaced concurrently, old file being erased, and
as such, the link operation is failing.

Patching with this very naive fix "solves" the issue (but this is
probably not something we want):

diff --git a/fs/namei.c b/fs/namei.c
index 209c51a5226c..befb15f4b865 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4231,9 +4231,10 @@ int vfs_link(struct dentry *old_dentry, struct
inode *dir, struct dentry *new_de

        inode_lock(inode);
        /* Make sure we don't allow creating hardlink to an unlinked file */
-       if (inode->i_nlink == 0 && !(inode->i_state & I_LINKABLE))
-               error =  -ENOENT;
-       else if (max_links && inode->i_nlink >= max_links)
+       //if (inode->i_nlink == 0 && !(inode->i_state & I_LINKABLE))
+       //      error =  -ENOENT;
+       // else
+       if (max_links && inode->i_nlink >= max_links)
                error = -EMLINK;
        else {
                error = try_break_deleg(inode, delegated_inode);

Kudos to Xavier Grand from Algolia for spotting the issue with a
reproducible case.

-- 
Xavier Roche -
xavier.roche at algolia.com

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: Possible FS race condition between vfs_rename and do_linkat (fs/namei.c)
  2019-08-24  7:24 Possible FS race condition between vfs_rename and do_linkat (fs/namei.c) Xavier Roche
@ 2019-08-24 10:01 ` Xavier Roche
  2019-08-26  9:36   ` Xavier Roche
  0 siblings, 1 reply; 3+ messages in thread
From: Xavier Roche @ 2019-08-24 10:01 UTC (permalink / raw)
  To: linux-fsdevel

Just a small addition:

> But maybe this is something the filesystem can not guarantee at all
> (w.r.t POSIX typically) ?

The POSIX standard does not seem to directly address this case, but my
understanding is that rename() atomicity should guarantee correct
behavior w.r.t rename()/link() concurrent operations:

See IEEE Std 1003.1,

(1) https://pubs.opengroup.org/onlinepubs/009695399/functions/rename.html

"If the link named by the new argument exists, it shall be removed and
old renamed to new. In this case, a link named new shall remain
visible to other processes throughout the renaming operation and refer
either to the file referred to by new or old before the operation
began."

"This rename() function is equivalent for regular files to that
defined by the ISO C standard. Its inclusion here expands that
definition to include actions on directories and specifies behavior
when the new parameter names a file that already exists. That
specification requires that the action of the function be atomic."

(2) https://pubs.opengroup.org/onlinepubs/009695399/functions/link.html
[ENOENT]
    "A component of either path prefix does not exist; the file named
by path1 does not exist; or path1 or path2 points to an empty string."

The only erroneous [ENOENT] case is when the file does not "exist";
but the renaming should guarantee that the file always "exists" ("a
link named new shall remain visible to other processes throughout the
renaming operation and refer either to the file referred to by new or
old before the operation began")

Cheers,

-- 
Xavier Roche

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Possible FS race condition between vfs_rename and do_linkat (fs/namei.c)
  2019-08-24 10:01 ` Xavier Roche
@ 2019-08-26  9:36   ` Xavier Roche
  0 siblings, 0 replies; 3+ messages in thread
From: Xavier Roche @ 2019-08-26  9:36 UTC (permalink / raw)
  To: linux-fsdevel

One more addition:

This issue seems to be a regression introduced by an old fix
(aae8a97d3ec30) in fs/namei.c preventing to hardlink a deleted file
(with i_nlink == 0):

Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Date:   Sat Jan 29 18:43:27 2011 +0530

    fs: Don't allow to create hardlink for deleted file

    Add inode->i_nlink == 0 check in VFS. Some of the file systems
    do this internally. A followup patch will remove those instance.
    This is needed to ensure that with link by handle we don't allow
    to create hardlink of an unlinked file. The check also prevent a race
    between unlink and link

    Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/namei.c b/fs/namei.c
index 83e92bab79a6..33be51a2ddb7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2906,7 +2906,11 @@ int vfs_link(struct dentry *old_dentry, struct
inode *dir, struct dentry *new_de
                return error;

        mutex_lock(&inode->i_mutex);
-       error = dir->i_op->link(old_dentry, dir, new_dentry);
+       /* Make sure we don't allow creating hardlink to an unlinked file */
+       if (inode->i_nlink == 0)
+               error =  -ENOENT;
+       else
+               error = dir->i_op->link(old_dentry, dir, new_dentry);
        mutex_unlock(&inode->i_mutex);
        if (!error)
                fsnotify_link(dir, inode, new_dentry);


Regards,

-- 
Xavier Roche -

^ permalink raw reply related	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-08-26  9:36 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-24  7:24 Possible FS race condition between vfs_rename and do_linkat (fs/namei.c) Xavier Roche
2019-08-24 10:01 ` Xavier Roche
2019-08-26  9:36   ` Xavier Roche

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).