/proc/kallsyms race vs module unload

/proc/kallsyms race vs module unload

[Alexey Dobriyan]

Steps to reproduce:

	while true; do modprobe xfs; rmmod xfs; done
		vs
	while true; do cat /proc/kallsyms >/dev/null; done


[where xfs could be any module, I haven't tried]

BUG: unable to handle kernel paging request at virtual address e19f808c
 printing eip:
c01dc361
*pde = 1ff5f067
*pte = 00000000
Oops: 0000 [#1]
PREEMPT
Modules linked in:
CPU:    0
EIP:    0060:[<c01dc361>]    Not tainted VLI
EFLAGS: 00010297   (2.6.21-rc3-8b9909ded6922c33c221b105b26917780cfa497d #2)
EIP is at vsnprintf+0x2af/0x48c
eax: e19f808c   ebx: ffffffff   ecx: e19f808c   edx: fffffffe
esi: dbe7aa84   edi: dbe2bf3c   ebp: ffffffff   esp: dbe2bec4
ds: 007b   es: 007b   fs: 00d8  gs: 0033  ss: 0068
Process cat (pid: 7242, ti=dbe2b000 task=df5790b0 task.ti=dbe2b000)
Stack: e19d6fde 00000000 00000010 00000008 ffffffff 00000001 00000598 dbe7aa68
       0002f362 00000010 dbe7b000 00000000 ffffffff c034bbe0 dbe7aa68 dfd31880
       dfa31e80 00001000 c01586b0 dbe2bf2c dbe2bf2c dfd31880 dfd31880 c01289f6
Call Trace:
 [<c01586b0>] seq_printf+0x2e/0x4b
 [<c01289f6>] s_show+0x4b/0x7f
 [<c0158c6e>] seq_read+0x196/0x278
 [<c0158ad8>] seq_read+0x0/0x278
 [<c0143c35>] vfs_read+0x72/0x93
 [<c0143f1c>] sys_read+0x41/0x67
 [<c0102486>] sysenter_past_esp+0x5f/0x85
 =======================
Code: 74 24 28 73 03 c6 06 20 46 4d 85 ed 7f f1 e9 b9 00 00 00 8b 0f 81 f9 ff 0f 00 00 b8 ea 45 36 c0 0f 46 c8 8b 54 24 30 89 c8 eb 06 <80> 38 00 74 07 40 4a 83 fa ff 75 f4 29 c8 89 c3 89 e8 f6 44 24
EIP: [<c01dc361>] vsnprintf+0x2af/0x48c SS:ESP 0068:dbe2bec4



What happens is that module_get_kallsym() drops module_mutex,
returns "struct module *", module unloaded, "struct module *"
used.

Re: /proc/kallsyms race vs module unload

[Paulo Marques]

Alexey Dobriyan wrote:
> [...]
> What happens is that module_get_kallsym() drops module_mutex,
> returns "struct module *", module unloaded, "struct module *"
> used.

The only use for the "struct module *" is to display the name of the 
module.

This can be solved by adding a "char mod_name[MODULE_NAME_LEN];" field 
to "kallsym_iter" and copy the name of the module over, while still 
holding module_mutex. It would be slightly slower, but safer.

We can even change the function's interface, so that it doesn't return a 
"struct module *" at all, since AFAICS kallsyms is the only user of that 
function.

It will still produce strange artifacts, though. If the iterator is 
already past the removed module symbols, it will skip as many symbols as 
the module symbol count, failing to show some symbols from unrelated 
modules. It won't oops, though.

I'll try to cook up a patch, if no one objects to this approach,

-- 
Paulo Marques - www.grupopie.com

"There cannot be a crisis today; my schedule is already full."

Re: /proc/kallsyms race vs module unload

[Alexey Dobriyan]

On Tue, Mar 13, 2007 at 06:49:50PM +0000, Paulo Marques wrote:
> Alexey Dobriyan wrote:
> >[...]
> >What happens is that module_get_kallsym() drops module_mutex,
> >returns "struct module *", module unloaded, "struct module *"
> >used.
>
> The only use for the "struct module *" is to display the name of the
> module.

Ehh?

> This can be solved by adding a "char mod_name[MODULE_NAME_LEN];" field
> to "kallsym_iter" and copy the name of the module over, while still
> holding module_mutex. It would be slightly slower, but safer.

	iter->owner = module_get_kallsym(iter->pos - kallsyms_num_syms,
					 &iter->value, &iter->type,
					 iter->name, sizeof(iter->name));
	if (iter->owner == NULL)
		return 0;

	/* Label it "global" if it is exported, "local" if not exported. */
	iter->type = is_exported(iter->name, iter->owner)
					     ^^^^^^^^^^^

Re: /proc/kallsyms race vs module unload

[Paulo Marques]

Alexey Dobriyan wrote:
> On Tue, Mar 13, 2007 at 06:49:50PM +0000, Paulo Marques wrote:
>> Alexey Dobriyan wrote:
>>> [...]
>>> What happens is that module_get_kallsym() drops module_mutex,
>>> returns "struct module *", module unloaded, "struct module *"
>>> used.
>> The only use for the "struct module *" is to display the name of the
>> module.
> 
> Ehh?
> 
>> This can be solved by adding a "char mod_name[MODULE_NAME_LEN];" field
>> to "kallsym_iter" and copy the name of the module over, while still
>> holding module_mutex. It would be slightly slower, but safer.
> 
> 	iter->owner = module_get_kallsym(iter->pos - kallsyms_num_syms,
> 					 &iter->value, &iter->type,
> 					 iter->name, sizeof(iter->name));
> 	if (iter->owner == NULL)
> 		return 0;
> 
> 	/* Label it "global" if it is exported, "local" if not exported. */
> 	iter->type = is_exported(iter->name, iter->owner)
> 					     ^^^^^^^^^^^

Yes, there is this "is_exported" call, but his can be moved completely 
into "module_get_kallsym" and have the "type" returned be already upper 
/ lower case.

That, together with filling the module name "module_get_kallsym()" would 
make the returned "struct module *" unneeded.

Since kallsyms is the only caller of that function, we can change its 
interface to not return a "struct module *" at all, and return just an 
integer that means "symbol found" or "no more symbols".

I'm still volunteering to do that patch, but you seem more active than 
me at the moment...

-- 
Paulo Marques
Software Development Department - Grupo PIE, S.A.
Phone: +351 252 290600, Fax: +351 252 290601
Web: www.grupopie.com