[Devel] Re: [PATCH RESEND 2/2] Fix some kallsyms_lookup() vs rmmod races

[Paulo Marques]

Ingo Molnar wrote:
> * Alexey Dobriyan <adobriyan at sw.ru> wrote:
> 
>> [cc'ing folks whose proc files are affected]
>>
>> kallsyms_lookup() can call module_address_lookup() which iterates over 
>> modules list without module_mutex taken. Comment at the top of 
>> module_address_lookup() says it's for oops resolution so races are 
>> irrelevant, but in some cases it's reachable from regular code:
> 
> looking at the problem from another angle: wouldnt this be something 
> that would benefit from freeze_processes()/unfreeze_processes(), and 
> hence no locking would be required?

I also considered this, but it seemed a little too "blunt" to stop 
everything (including completely unrelated processes and kernel threads) 
just to remove a module.

How about something like this instead?  (just for review)

It's a little more intrusive, since the interface for symbol lookup is 
changed (and it affects the callers), but it makes the caller aware that 
  it should set "safe_to_lock" if possible.

This way we avoid exposing module_mutex outside of module.c and avoid 
returning any data pointer to some data structure that might disappear 
before the caller tries to use it.

Some of these changes are actually cleanups, because the callers where 
creating a dummy modname variables that they didn't use afterwards.

The thing I like the less about this patch is the increase of stack 
usage on some code paths by about 60 bytes.

Anyway, I don't have very strong feelings about this, so if you think it 
would be better to use freeze_processes()/unfreeze_processes(), I can 
cook up a patch for that too...

-- 
Paulo Marques - www.grupopie.com

"Very funny Scotty. Now beam up my clothes."

diffstat:
>  arch/parisc/kernel/unwind.c |    3 --
>  arch/powerpc/xmon/xmon.c    |   11 ++++-----
>  arch/ppc/xmon/xmon.c        |    8 +++---
>  arch/sh64/kernel/unwind.c   |    4 +--
>  arch/x86_64/kernel/traps.c  |   10 ++++----
>  fs/proc/base.c              |    3 --
>  include/linux/kallsyms.h    |    6 +++-
>  include/linux/module.h      |   44 +++++++++++++++++++-----------------
>  kernel/kallsyms.c           |   53 +++++++++++++++++++++-----------------------
>  kernel/kprobes.c            |    6 ++--
>  kernel/lockdep.c            |    3 --
>  kernel/module.c             |   44 +++++++++++++++++++++++++++---------
>  kernel/time/timer_list.c    |    3 --
>  kernel/time/timer_stats.c   |    3 --
>  mm/slab.c                   |    6 ++--
>  15 files changed, 114 insertions(+), 93 deletions(-)

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch
URL: <http://lists.openvz.org/pipermail/devel/attachments/20070316/3ade19c3/attachment-0001.ksh>