Ocfs2 devel - Jun 2008 - [RFC] configfs: module reference counting rules

Hi,

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?

Thanks

Louis

-- 
Dr Louis Rilling			Kerlabs - IRISA
Skype: louis.rilling			Campus Universitaire de Beaulieu
Phone: (+33|0) 2 99 84 71 52		Avenue du General Leclerc
Fax: (+33|0) 2 99 84 71 71		35042 Rennes CEDEX - France
http://www.kerlabs.com/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: configfs-fix-module-refcount.patch
Type: text/x-diff
Size: 6835 bytes
Desc: not available
Url :
http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20080613/32facd75/attachment-0001.bin

On Fri, Jun 13, 2008 at 01:54:11AM +0200, Louis Rilling
wrote:
> 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.

Joel

-- 

Life's Little Instruction Book #232

	"Keep your promises."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127