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

[Rusty Russell <rusty@rustcorp.com.au>]

In message <20020921033830.A32446@arizona.localdomain> you write:
> 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.)

But in practice there are many resources which are only unregistered
in the "unloading module" case: certainly by far the most common
case.  It's hard for most module authors to spot this kind of race.

> 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;							\
> 	})

Whack me with a cacheline... that's going to hurt on SMP.  You could
use a brlock, though.

You're assuming security methods cannot sleep?  If true, use a brlock
and be done with it.  Otherwise, you'll need a refcount, and we're
back to bigrefs and synchronize_kernel.  Adding a bigref to each
security_ops struct might be acceptable, since it's so big.  Adding a
single "owner" field certainly is.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.