kernfs: invoke kernfs_unmap_bin_file() directly from __kernfs_remove()

kernfs_unmap_bin_file() is supposed to unmap all memory mappings of
the target file before kernfs_remove() finishes; however, it currently
is being called from kernfs_addrm_finish() and has the same race
problem as the original implementation of deactivation when there are
multiple removers - only the remover which snatches the node to its
addrm_cxt->removed list is guaranteed to wait for its completion
before returning.

It can be fixed by moving kernfs_unmap_bin_file() invocation from
kernfs_addrm_finish() to __kernfs_remove().  The function may be
called multiple times but that shouldn't do any harm.

We end up dropping kernfs_mutex in the removal loop and the node may
be removed inbetween by someone else.  kernfs_unlink_sibling() is
updated to test whether the node has already been removed and return
accordingly.  __kernfs_remove() in turn performs post-unlinking
cleanup only if it actually unlinked the node.

KERNFS_HAS_MMAP test is moved out of the unmap function into
__kernfs_remove() so that we don't unlock kernfs_mutex unnecessarily.
While at it, drop the now meaningless "bin" qualifier from the
function name.

v2: Rewritten to fit the v2 restructuring of removal path.  HAS_MMAP
    test relocated.

Signed-off-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

3 files changed, 35 insertions, 14 deletions

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index e565ec096ae9..f878e4f2efe7 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -121,13 +121,17 @@ static int kernfs_link_sibling(struct kernfs_node *kn)
  *      Locking:
  *      mutex_lock(kernfs_mutex)
  */
-static void kernfs_unlink_sibling(struct kernfs_node *kn)
+static bool kernfs_unlink_sibling(struct kernfs_node *kn)
 {
+        if (RB_EMPTY_NODE(&kn->rb))
+                return false;
+
         if (kernfs_type(kn) == KERNFS_DIR)
                 kn->parent->dir.subdirs--;
 
         rb_erase(&kn->rb, &kn->parent->dir.children);
         RB_CLEAR_NODE(&kn->rb);
+        return true;
 }
 
 /**
@@ -496,7 +500,6 @@ void kernfs_addrm_finish(struct kernfs_addrm_cxt *acxt)
 
                 acxt->removed = kn->u.removed_list;
 
-                kernfs_unmap_bin_file(kn);
                 kernfs_put(kn);
         }
 }
@@ -854,23 +857,44 @@ static void __kernfs_remove(struct kernfs_addrm_cxt *acxt,
 
         /* unlink the subtree node-by-node */
         do {
-                struct kernfs_iattrs *ps_iattr;
-
                 pos = kernfs_leftmost_descendant(kn);
 
-                if (pos->parent) {
-                        kernfs_unlink_sibling(pos);
+                /*
+                 * We're gonna release kernfs_mutex to unmap bin files,
+                 * Make sure @pos doesn't go away inbetween.
+                 */
+                kernfs_get(pos);
+
+                /*
+                 * This must be come before unlinking; otherwise, when
+                 * there are multiple removers, some may finish before
+                 * unmapping is complete.
+                 */
+                if (pos->flags & KERNFS_HAS_MMAP) {
+                        mutex_unlock(&kernfs_mutex);
+                        kernfs_unmap_file(pos);
+                        mutex_lock(&kernfs_mutex);
+                }
+
+                /*
+                 * kernfs_unlink_sibling() succeeds once per node.  Use it
+                 * to decide who's responsible for cleanups.
+                 */
+                if (!pos->parent || kernfs_unlink_sibling(pos)) {
+                        struct kernfs_iattrs *ps_iattr =
+                                pos->parent ? pos->parent->iattr : NULL;
 
                         /* update timestamps on the parent */
-                        ps_iattr = pos->parent->iattr;
                         if (ps_iattr) {
                                 ps_iattr->ia_iattr.ia_ctime = CURRENT_TIME;
                                 ps_iattr->ia_iattr.ia_mtime = CURRENT_TIME;
                         }
+
+                        pos->u.removed_list = acxt->removed;
+                        acxt->removed = pos;
                 }
 
-                pos->u.removed_list = acxt->removed;
-                acxt->removed = pos;
+                kernfs_put(pos);
         } while (pos != kn);
 }
 
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 231a171f48b6..404ffd2f27bc 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -700,14 +700,11 @@ static int kernfs_fop_release(struct inode *inode, struct file *filp)
         return 0;
 }
 
-void kernfs_unmap_bin_file(struct kernfs_node *kn)
+void kernfs_unmap_file(struct kernfs_node *kn)
 {
         struct kernfs_open_node *on;
         struct kernfs_open_file *of;
 
-        if (!(kn->flags & KERNFS_HAS_MMAP))
-                return;
-
         spin_lock_irq(&kernfs_open_node_lock);
         on = kn->attr.open;
         if (on)
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 57a93f4d645c..e9ec38c86074 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -113,7 +113,7 @@ struct kernfs_node *kernfs_new_node(struct kernfs_root *root, const char *name,
  */
 extern const struct file_operations kernfs_file_fops;
 
-void kernfs_unmap_bin_file(struct kernfs_node *kn);
+void kernfs_unmap_file(struct kernfs_node *kn);
 
 /*
  * symlink.c