[Devel] [PATCH RH7 0/3] fix crash on module reference leak

[Kirill Tkhai]

On 26.02.2018 13:01, Pavel Tikhomirov wrote:
> That's how the race happens:
> ========
> mutex_lock(&module_mutex);
> try_stop_module -> stop_machine -> __stop_machine -> stop_cpus ->
> __stop_cpus->[for_each_cpu]cpu_stop_queue_work ... -> __try_stop_module
> {
> 	if (module_refcount
> 	    {
> 		count decs;
> 		smp_rmb;
> 		count incs;
> 		return incs - decs;
> 	})
> 		return -EWOULDBLOCK;
> 
> #2 decs == incs, no reference on module
> 	
> 	state = MODULE_STATE_GOING;
> #3 change state to GOING away
> }
> mutex_unlock(&module_mutex);
> 
> ========
> uio_open
> {
> 	try_module_get
> 	{
> 		preempt_disable(); // only compiler barrier
> 		if (module_is_live {state != MODULE_STATE_GOING})
> #1 load and check state is not GOING
> 
> 		{
> 			increment incs;
> #4 increment while already GOING
> 		}
> 		preempt_enable();
> 	};
> }

1)stop_machine() implies smp_mb() on all cpus.
2)barrier() implies smp_rmb() on x86.

So, this race should not happen.

Kirill

> commit 24a2b6e22b38 fixes it by using atomic_inc_not_zero in
> try_module_get, thus either #4 can't happen if we already released
> module reference in try_stop_module, or we can't release the reference
> in try_stop_module if try_module_get already got it's reference on the
> module.
> 
> Note: These patches are cherry-picked from MS with two small conflicts:
> 1) In second patch rhel-introduced structure changes hunk's surounding,
> but no meaningful change here.
> 2) In third patch leave stop_machine header, it is still used elsewhere.
> 
> https://jira.sw.ru/browse/PSBM-80508
> 
> Christoph Lameter (1):
>   modules: use raw_cpu_write for initialization of per cpu refcount.
> 
> Masami Hiramatsu (2):
>   module: Replace module_ref with atomic_t refcnt
>   module: Remove stop_machine from module unloading
> 
>  include/linux/module.h        | 16 +-------
>  include/trace/events/module.h |  2 +-
>  kernel/module.c               | 93 ++++++++++++++++++-------------------------
>  3 files changed, 40 insertions(+), 71 deletions(-)
>