configfs: module reference counting rules

I'm a bit confused by configfs module reference counting, and I feel that it does not really protect against module unloading. If my feeling is correct, I'd suggest to change module reference counting rules in configfs, for instance like in the attached patch. If my feeling is wrong, could someone shed some light?

Quoted 5 lines · Louis Rilling

I'm a bit confused by configfs module reference counting, and I feel that it does not really protect against module unloading. If my feeling is correct, I'd suggest to change module reference counting rules in configfs, for instance like in the attached patch. If my feeling is wrong, could someone shed some light?

You're wrong, sort of :-) I worked quite hard on this, so I was
scared you'd found something - you haven't.
configfs is only responsible for its *own* references on the
client module. The client module is responsible for protecting itself.
What do I mean? In mkdir(), the problem is racing
configfs_unregister_subsystem(). The group *has* to be live, because we have i_mutex - unregister_subsystem can't tear down the directory until we release it. So we're safe to call ->make_item/group(). After we've done that, we have a type, and we can try_module_get(). If someone else is in unregister_subsystem, that fails and we clean up. If not, we have a reference and they can't unload.

This is hard logic, and not something we want each and every
client module to have to figure out. Your change has them explicitly __module_get() in ->make_item/group(), which isn't safe because of this race!

What about attributes? They can only be accessed via the
filesystem exposure. If they are in the filesystem, unregister can't happen. If they are opened, the module refcount is incremented.
Your big concern came from rmdir(). Specifically, while it was
safe to allocate an object during ->make_item/group(), what happens after ->drop_item() is called and then module_put()? If the item has a reference count still, the ->release() is not called. We may be dropping our last reference on the module, and now the module can be unloaded. This is the module's problem, not ours! configfs no longer has a reference to the module, and thus cannot control its lifetime.

Anyone who has just the single reference is safe, because that's
the last reference. When we call the last config_item_put(), the release happens. That's the simple case.
A module that takes an additional reference to the config item
needs to have this protection in place. All in-kernel users take and release items in one function call. They don't hold long-term references. If they did, they'd have to have a way of ensuring their structure remained alive - and this would be the case if configfs wasn't even involved.

On Thu, Jun 12, 2008 at 08:33:12PM -0700, Joel Becker wrote:
Ok, got it. The race is between unregister_subsystem() and mkdir() at the root of the subsystem (or one of its default groups). In deeper, user-created groups, this would be a design bug of the subsystem if this race could occur.

Quoted 4 lines

This is hard logic, and not something we want each and every
client module to have to figure out. Your change has them explicitly __module_get() in ->make_item/group(), which isn't safe because of this race!

Well, this remains hard logic for the modules. But I agree that they should not impact the logic that deals with racing mkdir() and unregister_subsystem().

Sure. I was thinking that configfs helped subsystems with such module reference counting issues, but I was wrong.

Thanks for these explanations.

Quoted 4 lines · Louis Rilling

On Thu, Jun 12, 2008 at 08:33:12PM -0700, Joel Becker wrote:
Ok, got it. The race is between unregister_subsystem() and mkdir() at the root of the subsystem (or one of its default groups). In deeper, user-created groups, this would be a design bug of the subsystem if this race could occur.

Exactly.

Quoted 8 lines

Quoted 5 lines

This is hard logic, and not something we want each and every
client module to have to figure out. Your change has them explicitly __module_get() in ->make_item/group(), which isn't safe because of this race!

Well, this remains hard logic for the modules. But I agree that they should not impact the logic that deals with racing mkdir() and unregister_subsystem().

It is, but most modules don't have to deal with it. Most
modules (all in-kernel configfs users) have a single refrence on it. When they take an additional one, it's for the duration of a function call. They have to make sure that it's safe to call the function in the first place, so it's clearly safe to get/put the item.
Forget about the configfs view that is presented to userspace.
If you were a module with inter-linked structures, you'd have to make sure they were freed cleanly. For simple things, you create and drop them. If a module has a complex interlinkage, they have a mechanism to handle it.

Example: filesystems can hang whatever they want off of VFS
structures like inodes and superblocks - a mounted filesystem prevents rmmod. Everything is safe, *everything*, as long as it is all cleaned up when put_super() returns.
So for the simple case, we provide plenty of protection. If
someone wants to do something fancier, they have to provide their own protection, but they would anyway. And we can't know their complex lifecycle, so we can't really help anyway.

1/ taking the module reference is only needed if mkdir() is called under a subsystem root or one of its default group, right? Of course this is a bit complex to check for this condition, and it does not hurt to take module references in all cases of mkdir().

2/ this module reference counting makes unregister_subsystem() win against mkdir(), but only if unregister_subsystem() is called in module_cleanup(), because otherwise try_module_get() would succeed, right? If so, this means that after having called register_subsystem(), a module_init() cannot cleanly fail. Perhaps this should be documented in that case.

3/ to make unregister_subsystem() win against mkdir(), mkdir() should try_module_get() on the subsystem's owner, not on the new item owner (as is done by the current code), right? Maybe there is a bug here...

Quoted 4 lines

So for the simple case, we provide plenty of protection. If
someone wants to do something fancier, they have to provide their own protection, but they would anyway. And we can't know their complex lifecycle, so we can't really help anyway.

Actually, I'm developing a framework based on configfs, with which many modules can be linked together through mkdir() and symlink() operations. So I'm already managing such reference holding, but the fact that configfs does not hold a reference until the last config_item_put() imposes limitations (with which I can live though) to the framework:

For instance, A is a group of the framework, and mkdir A/B creates an
object implemented by module "mod_b". Before calling mod_b's constructor, A's make_item() has to grab a reference on mod_b, and fail if not successful. Then this reference cannot be dropped before B's last reference is dropped, which can happen a long time after rmdir A/B. So A's drop_item() cannot drop mod_b's reference, and A has to provide the release() config_item_operation for B, which will call B's destructor and finally drop mod_b's reference.

Quoted 4 lines · Louis Rilling

1/ taking the module reference is only needed if mkdir() is called under a subsystem root or one of its default group, right? Of course this is a bit complex to check for this condition, and it does not hurt to take module references in all cases of mkdir().

Correct, but it's much cleaner to always take the module ref.

Quoted 6 lines

2/ this module reference counting makes unregister_subsystem() win against mkdir(), but only if unregister_subsystem() is called in module_cleanup(), because otherwise try_module_get() would succeed, right? If so, this means that after having called register_subsystem(), a module_init() cannot cleanly fail. Perhaps this should be documented in that case.

Correct again, mkdir can race a failing module_init(). This is
the same as register_filesystem(). A module that needed to protect against this could have a mutex they release right before module_init() succeeds. They'd check it in make_item(). But most everyone can safely make configfs_register_subsystem() the last thing in their module_init().

Quoted 4 lines

3/ to make unregister_subsystem() win against mkdir(), mkdir() should try_module_get() on the subsystem's owner, not on the new item owner (as is done by the current code), right? Maybe there is a bug here...

Nope. You can build a subsystem out of multiple modules if you
like. The module that owns the newly created object needs to be pinned, and that's new_item->type->owner. If a subsystem lives within one module, then subsys->type->owner == new_item->type->owner, and it doesn't matter.

Quoted 11 lines

Quoted 5 lines

So for the simple case, we provide plenty of protection. If
someone wants to do something fancier, they have to provide their own protection, but they would anyway. And we can't know their complex lifecycle, so we can't really help anyway.

Actually, I'm developing a framework based on configfs, with which many modules can be linked together through mkdir() and symlink() operations. So I'm already managing such reference holding, but the fact that configfs does not hold a reference until the last config_item_put() imposes limitations (with which I can live though) to the framework:

Think about it this way: the try_module_get() isn't to protect
the client module, it is to protect configfs. It makes sure that if someone calls a VFS operation on a configfs inode, configfs can follow the config_*_operations safely. Once a config_item is removed from the filesystem view, configfs is done with it.
Look at it the other way around. A config_item is not a
structure owned by configfs. It is a part of the larger object, which is owned by the module that created it. The config_item portion just lets configfs display it to userspace.

Quoted 9 lines

For instance, A is a group of the framework, and mkdir A/B creates an
object implemented by module "mod_b". Before calling mod_b's constructor, A's make_item() has to grab a reference on mod_b, and fail if not successful. Then this reference cannot be dropped before B's last reference is dropped, which can happen a long time after rmdir A/B. So A's drop_item() cannot drop mod_b's reference, and A has to provide the release() config_item_operation for B, which will call B's destructor and finally drop mod_b's reference.

	Wow, so A->make_item() does something like:

    submod = lookup_which_mod();
    if (!strcmp(submod, "mod_a"))
        new_thing = mod_a->alloc();
    else if (!strcmp(submod, "mod_b"))
        new_thing = mod_b->alloc();

    return &new_thing->config_item;

That's pretty complicated, I agree. But certainly doable.

	Why can't mod_b provide a ->release() that does

module_put(self)?

Or are you trying to hide that detail from the person
who is implementing mod_b? Even better, use the chained release scheme that is used by bio_endio(). Have mod_b control its own config_item_operations. In make_item, copy off the type and operations, then put in your own. In drop, put them back.

struct my_item_type {
	struct config_item_type *original_type;
	struct config_item_type type;
	struct config_item_operations ops;
};

make_item()

{
	mod_b = alloc_mod_b();
	my_type = kzalloc(struct my_item_type);
	my_type->original_type = mod_b->item->ci_type;
	my_type->ops = *my_type->original_type->ct_item_ops;
	my_type->ops.release = my_item_release();
	my_type->type = *my_type->original_type;
	my_type->type.ops = &my_type->ops;
	mod_b->item->ci_type = &my_type->type;

	return &mod_b->item;
}

my_item_release(struct config_item *item)

{
	my_type = to_my_type(item->type);
	item->ci_type = my_type->original_type;
	kfree(my_type);
	item->ci_type->ct_ops->release(item);
}

On Sat, Jun 14, 2008 at 01:47:01AM -0700, Joel Becker wrote:
IMHO, what really hurts configfs is that the unregister_subsystem() vs
mkdir() race is not solved unless mkdir() tries to grab a reference on the subsystem's module. And the current code of mkdir() does not ensure that in the "several modules" case.

I do something like this (and this works):

Quoted 2 lines

	Why can't mod_b provide a ->release() that does

module_put(self)?

Because this is simply wrong. Doing module_put(self) exposes the modules's function to be run while another cpu unloads the module. Note how I solve this

Quoted 5 lines

Quoted 2 lines

	Why can't mod_b provide a ->release() that does

module_put(self)?

Because this is simply wrong. Doing module_put(self) exposes the modules's function to be run while another cpu unloads the module. Note how I solve this

How so? As long as the module_put() is the last thing, you're
fine. That said, we both have better solutions with our wrappered functions.

With a preemptible kernel, after module_put(self) the few assembly instructions cleaning up the stack before returning to the caller can be called after the memory allocated for the module has been freed. Which will make the kernel crash.