Re: [PATCH] In-kernel module loader 1/7

[Kevin O'Connor]

On Fri, Sep 20, 2002 at 11:22:08AM +1000, Rusty Russell wrote:
> Well, it's up to you.  You *could* implement:
> 
> #define call_security(method , ...)					\
> 	({ int __ret;							\
> 	   if (try_module_get(security_ops->owner)) {			\
> 		__ret = security_ops->method(__VA_ARGS__);		\
> 		module_put(security_ops->owner);			\
> 	   } else							\
> 		/* If unloading or loading, default to "allow" */	\
> 		__ret = 0;						\
> 	  __ret;							\
> 	})
[...]
> Now, if you don't have CONFIG_MODULES this becomes the code as it is
> now.

Hi Rusty,

Please consider the following non-module code snippet:

int
sys_enable_foo_security()
{
        foocache = kmalloc(100000);
        register_security(&foo_ops);
}

int
sys_disable_foo_security()
{
        unregister_security(&foo_ops);
        kfree(foocache);  // OOPS
}

If I follow Roman's argument correctly, the unload race is not module
specific.  (The problem is that unregister_security() only asserts that no
new callers will be made to foo_ops, it doesn't guarantee that there are no
current callers.)

In the above example, one solution would be to reference count foocache.
However, another viable solution would be to ref-count the security_ops
field.

Anyway, given that the problem is a general resource management issue (and
not module specific), I think one could implement call_security() with less
overhead:

#define call_security(method , ...)					\
	({ int __ret;							\
	   read_lock(&SecurityLock);                                    \
	   __ret = security_ops->method(__VA_ARGS__);                   \
           read_unlock(&SecurityLock);                                  \
	  __ret;							\
	})

where (un)register_security used a write_lock to guard accesses to
security_ops changes.

This implementation is still a bit sluggish (as well as limiting), however
one could conceivable use RCU or a similar mechanism to further reduce
overhead of the common path.

-Kevin

P.S. it may also be possible for this alternate solution to work:

#define call_security(method , ...)					\
	({ int __ret;							\
           atomic_inc(&SecurityRefCount);                               \
	   __ret = security_ops->method(__VA_ARGS__);                   \
           atomic_dec(&SecurityRefCount);                               \
	})

where unregister_security set the security_ops field to a dummy value and
then waited for the ref-count to hit zero before returning.  However, this
may depend too heavily on memory ordering..

-- 
 ------------------------------------------------------------------------
 | Kevin O'Connor                     "BTW, IMHO we need a FAQ for      |
 | [e-mail]                  'IMHO', 'FAQ', 'BTW', etc. !"    |
 ------------------------------------------------------------------------