configfs: make module reference counting robust Module references taken by configfs at mkdir() unfortunately do not ensure that the module implementing an item will remain loaded before the item is released, because 1/ the reference is taken after having created the item, and 2/ the reference is dropped when dropping the item, which may be before the item's last reference count is dropped. For instance this can lead to calling the release item operation when the module implementing the function is already unloaded. This patch changes module reference counting rules in configfs to ensure that for each item appearing in configfs a module reference is held until the item is released. For each item/group/default group created in make_item()/make_group(), the subsystem is responsible for grabbing a reference on the matching modules. configfs will drop that reference after having released the item/group/default group. Something similar might be needed for configfs_attribute as well. Signed-off-by: Louis Rilling --- drivers/net/netconsole.c | 1 + fs/configfs/dir.c | 17 +++++++---------- fs/configfs/item.c | 5 +++++ fs/dlm/config.c | 7 +++++++ fs/ocfs2/cluster/heartbeat.c | 1 + fs/ocfs2/cluster/nodemanager.c | 5 +++++ 6 files changed, 26 insertions(+), 10 deletions(-) Index: b/drivers/net/netconsole.c =================================================================== --- a/drivers/net/netconsole.c 2008-06-13 01:24:55.000000000 +0200 +++ b/drivers/net/netconsole.c 2008-06-13 01:25:51.000000000 +0200 @@ -608,6 +608,7 @@ static struct config_item *make_netconso memset(nt->np.remote_mac, 0xff, ETH_ALEN); /* Initialize the config_item member */ + __module_get(netconsole_target_type.ct_owner); config_item_init_type_name(&nt->item, name, &netconsole_target_type); /* Adding, but it is disabled */ Index: b/fs/configfs/dir.c =================================================================== --- a/fs/configfs/dir.c 2008-06-13 00:41:49.000000000 +0200 +++ b/fs/configfs/dir.c 2008-06-13 01:26:13.000000000 +0200 @@ -1080,18 +1080,18 @@ static int configfs_mkdir(struct inode * goto out_unlink; } + /* + * make_item()/make_group() should have grabbed a module reference for item + * Let's check it but do not keep one module reference more. The + * reference for item will be dropped after item is released. + */ owner = type->ct_owner; if (!try_module_get(owner)) { + printk(KERN_ERR "configfs: Ill-behaved subsystem: module reference count may be broken\n"); ret = -EINVAL; goto out_unlink; } - - /* - * I hate doing it this way, but if there is - * an error, module_put() probably should - * happen after any cleanup. - */ - module_got = 1; + module_put(owner) if (group) ret = configfs_attach_group(parent_item, item, dentry); @@ -1111,9 +1111,6 @@ out_unlink: client_drop_item(parent_item, item); mutex_unlock(&subsys->su_mutex); - - if (module_got) - module_put(owner); } out_put: Index: b/fs/configfs/item.c =================================================================== --- a/fs/configfs/item.c 2008-06-13 00:57:53.000000000 +0200 +++ b/fs/configfs/item.c 2008-06-13 00:59:42.000000000 +0200 @@ -153,13 +153,18 @@ static void config_item_cleanup(struct c struct config_item_type * t = item->ci_type; struct config_group * s = item->ci_group; struct config_item * parent = item->ci_parent; + struct module *owner = NULL; pr_debug("config_item %s: cleaning up\n",config_item_name(item)); if (item->ci_name != item->ci_namebuf) kfree(item->ci_name); item->ci_name = NULL; + if (t) + owner = t->ct_owner; if (t && t->ct_item_ops && t->ct_item_ops->release) t->ct_item_ops->release(item); + if (owner) + module_put(owner); if (s) config_group_put(s); if (parent) Index: b/fs/dlm/config.c =================================================================== --- a/fs/dlm/config.c 2008-06-13 01:02:35.000000000 +0200 +++ b/fs/dlm/config.c 2008-06-13 01:21:33.000000000 +0200 @@ -408,6 +408,9 @@ static struct config_group *make_cluster if (!cl || !gps || !sps || !cms) goto fail; + __module_get(cluster_type.ct_owner); + __module_get(spaces_type.ct_owner); + __module_get(comms_type.ct_owner); config_group_init_type_name(&cl->group, name, &cluster_type); config_group_init_type_name(&sps->ss_group, "spaces", &spaces_type); config_group_init_type_name(&cms->cs_group, "comms", &comms_type); @@ -479,6 +482,8 @@ static struct config_group *make_space(s if (!sp || !gps || !nds) goto fail; + __module_get(space_type.ct_owner); + __module_get(nodes_type.ct_owner); config_group_init_type_name(&sp->group, name, &space_type); config_group_init_type_name(&nds->ns_group, "nodes", &nodes_type); @@ -530,6 +535,7 @@ static struct config_item *make_comm(str if (!cm) return NULL; + __module_get(comm_type.ct_owner); config_item_init_type_name(&cm->item, name, &comm_type); cm->nodeid = -1; cm->local = 0; @@ -563,6 +569,7 @@ static struct config_item *make_node(str if (!nd) return NULL; + __module_get(node_type.ct_owner); config_item_init_type_name(&nd->item, name, &node_type); nd->nodeid = -1; nd->weight = 1; /* default weight of 1 if none is set */ Index: b/fs/ocfs2/cluster/heartbeat.c =================================================================== --- a/fs/ocfs2/cluster/heartbeat.c 2008-06-13 01:23:34.000000000 +0200 +++ b/fs/ocfs2/cluster/heartbeat.c 2008-06-13 01:24:19.000000000 +0200 @@ -1499,6 +1499,7 @@ static struct config_item *o2hb_heartbea if (reg == NULL) goto out; /* ENOMEM */ + __module_get(o2hb_region_type.ct_owner); config_item_init_type_name(®->hr_item, name, &o2hb_region_type); ret = ®->hr_item; Index: b/fs/ocfs2/cluster/nodemanager.c =================================================================== --- a/fs/ocfs2/cluster/nodemanager.c 2008-06-13 01:09:29.000000000 +0200 +++ b/fs/ocfs2/cluster/nodemanager.c 2008-06-13 01:23:05.000000000 +0200 @@ -718,6 +718,7 @@ static struct config_item *o2nm_node_gro goto out; /* ENOMEM */ strcpy(node->nd_name, name); /* use item.ci_namebuf instead? */ + __module_get(o2nm_node_type.ct_owner); config_item_init_type_name(&node->nd_item, name, &o2nm_node_type); spin_lock_init(&node->nd_lock); @@ -831,6 +832,10 @@ static struct config_group *o2nm_cluster if (cluster == NULL || ns == NULL || o2hb_group == NULL || defs == NULL) goto out; + if (!try_module_get(o2hb_group->cg_item.ci_type->ct_owner)) + goto out; + __modult_get(o2nm_cluster_type.ct_owner); + __modult_get(o2nm_node_group_type.ct_owner); config_group_init_type_name(&cluster->cl_group, name, &o2nm_cluster_type); config_group_init_type_name(&ns->ns_group, "node",