configfs: configfs_mkdir() failed to cleanup linkage.

If configfs_mkdir() errored in certain ways after the parent<->child
linkage was already created, it would not undo the linkage.  Also,
comment the reference counting for clarity.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
Signed-off-by: Mark Fasheh <mark.fasheh@oracle.com>

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 810c139..5f95218 100644 (file)
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -505,13 +505,15 @@ static int populate_groups(struct config_group *group)
        int i;
 
        if (group->default_groups) {
-               /* FYI, we're faking mkdir here
+               /*
+                * FYI, we're faking mkdir here
                 * I'm not sure we need this semaphore, as we're called
                 * from our parent's mkdir.  That holds our parent's
                 * i_mutex, so afaik lookup cannot continue through our
                 * parent to find us, let alone mess with our tree.
                 * That said, taking our i_mutex is closer to mkdir
-                * emulation, and shouldn't hurt. */
+                * emulation, and shouldn't hurt.
+                */
                mutex_lock(&dentry->d_inode->i_mutex);
 
                for (i = 0; group->default_groups[i]; i++) {
@@ -546,20 +548,34 @@ static void unlink_obj(struct config_item *item)
 
                item->ci_group = NULL;
                item->ci_parent = NULL;
+
+               /* Drop the reference for ci_entry */
                config_item_put(item);
 
+               /* Drop the reference for ci_parent */
                config_group_put(group);
        }
 }
 
 static void link_obj(struct config_item *parent_item, struct config_item *item)
 {
-       /* Parent seems redundant with group, but it makes certain
-        * traversals much nicer. */
+       /*
+        * Parent seems redundant with group, but it makes certain
+        * traversals much nicer.
+        */
        item->ci_parent = parent_item;
+
+       /*
+        * We hold a reference on the parent for the child's ci_parent
+        * link.
+        */
        item->ci_group = config_group_get(to_config_group(parent_item));
        list_add_tail(&item->ci_entry, &item->ci_group->cg_children);
 
+       /*
+        * We hold a reference on the child for ci_entry on the parent's
+        * cg_children
+        */
        config_item_get(item);
 }
 
@@ -684,6 +700,10 @@ static void client_drop_item(struct config_item *parent_item,
        type = parent_item->ci_type;
        BUG_ON(!type);
 
+       /*
+        * If ->drop_item() exists, it is responsible for the
+        * config_item_put().
+        */
        if (type->ct_group_ops && type->ct_group_ops->drop_item)
                type->ct_group_ops->drop_item(to_config_group(parent_item),
                                                item);
@@ -694,14 +714,14 @@ static void client_drop_item(struct config_item *parent_item,
 
 static int configfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 {
-       int ret;
+       int ret, module_got = 0;
        struct config_group *group;
        struct config_item *item;
        struct config_item *parent_item;
        struct configfs_subsystem *subsys;
        struct configfs_dirent *sd;
        struct config_item_type *type;
-       struct module *owner;
+       struct module *owner = NULL;
        char *name;
 
        if (dentry->d_parent == configfs_sb->s_root) {
@@ -754,43 +774,63 @@ static int configfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 
        kfree(name);
        if (!item) {
+               /*
+                * If item == NULL, then link_obj() was never called.
+                * There are no extra references to clean up.
+                */
                ret = -ENOMEM;
                goto out_put;
        }
 
-       ret = -EINVAL;
+       /*
+        * link_obj() has been called (via link_group() for groups).
+        * From here on out, errors must clean that up.
+        */
+
        type = item->ci_type;
-       if (type) {
-               owner = type->ct_owner;
-               if (try_module_get(owner)) {
-                       if (group) {
-                               ret = configfs_attach_group(parent_item,
-                                                           item,
-                                                           dentry);
-                       } else {
-                               ret = configfs_attach_item(parent_item,
-                                                          item,
-                                                          dentry);
-                       }
+       if (!type) {
+               ret = -EINVAL;
+               goto out_unlink;
+       }
 
-                       if (ret) {
-                               down(&subsys->su_sem);
-                               if (group)
-                                       unlink_group(group);
-                               else
-                                       unlink_obj(item);
-                               client_drop_item(parent_item, item);
-                               up(&subsys->su_sem);
+       owner = type->ct_owner;
+       if (!try_module_get(owner)) {
+               ret = -EINVAL;
+               goto out_unlink;
+       }
 
-                               module_put(owner);
-                       }
-               }
+       /*
+        * I hate doing it this way, but if there is
+        * an error,  module_put() probably should
+        * happen after any cleanup.
+        */
+       module_got = 1;
+
+       if (group)
+               ret = configfs_attach_group(parent_item, item, dentry);
+       else
+               ret = configfs_attach_item(parent_item, item, dentry);
+
+out_unlink:
+       if (ret) {
+               /* Tear down everything we built up */
+               down(&subsys->su_sem);
+               if (group)
+                       unlink_group(group);
+               else
+                       unlink_obj(item);
+               client_drop_item(parent_item, item);
+               up(&subsys->su_sem);
+
+               if (module_got)
+                       module_put(owner);
        }
 
 out_put:
        /*
-        * link_obj()/link_group() took a reference from child->parent.
-        * Drop our working ref
+        * link_obj()/link_group() took a reference from child->parent,
+        * so the parent is safely pinned.  We can drop our working
+        * reference.
         */
        config_item_put(parent_item);