Re: [RFC PATCH] module: debugging check for runaway kthreads

[Dmitry Antipov <dmitry.antipov@linaro.org>]

On 02/29/2012 12:43 AM, Andrew Morton wrote:

> Please make the stub function a proper C function, not a macro.  It
> provides type checking, can prevent compile warnings and is generally
> easier on the eyes.

OK

> I think this should be under the kernel hacking menu, dependent on
> CONFIG_DEBUG_KERNEL, in lib/Kconfig.debug.

I tried to group all module options together. Nevertheless,
I have no objections for separately grouping debugging options.

>>   struct kthread {
>>   	int should_stop;
>> +#ifdef CONFIG_MODULE_KTHREAD_CHECK
>> +	void *fn;
>> +#endif
>
> A little comment describing what this field is for would be nice.

OK

>> +unsigned long get_kthread_func(struct task_struct *tsk)
>> +{
>> +	struct kthread *kt;
>> +	unsigned long addr;
>> +
>> +	get_task_struct(tsk);
>> +	BUG_ON(!(tsk->flags&  PF_KTHREAD));
>> +	kt = to_kthread(tsk);
>> +	barrier();
>> +	addr = tsk->vfork_done ? (unsigned long)kt->fn : 0UL;
>> +	put_task_struct(tsk);
>> +	return addr;
>> +}
>
> gack, I hadn't noticed the kthread ab^wre^wuse of vfork_done before.
> Kooky.  Undocumented, too.

Hm... vfork_done of the just created kthread is NULL. When the new
kthread calls kthread(), it assigns it's private 'struct kthread' to
it's own tsk->vfork_done (IIUC, this is just a hack to avoid ugly
stuff like excessive pointer in task_struct or pointer type conversion).
So, to_kthread(tsk) is valid only if tsk->vfork_done is non-NULL,
otherwise it's just a bogus pointer. IMHO this hack should be documented
in kthread().

>> +#ifdef CONFIG_KALLSYMS
>> +static const char *get_ksymbol(struct module *mod, unsigned long addr,
>> +			       unsigned long *size, unsigned long *offset);
>> +#else
>> +#define get_ksymbol(mod, addr, size, offset) NULL
>> +#endif
>
> Can this code block be moved to after the get_ksymbol() definition so
> the forward declaration is unneeded?

OK

> Did we really need to use the internal get_ksymbol(), rather than an
> official kallsyms interface function?

I don't see a kallsyms interface function which is able to look through
just one module. Since the module name (and struct module pointer) is
known, it looks redundant to lookup through the whole kallsyms tables.

> The CONFIG_KALLSYMS=n stub should be written in C.  Make it return
> "<unknown>" and the ?: in check_kthreads() can be done away with.

OK

>> +#else
>> +
>> +#define check_kthreads(mod) do { } while (0)
>
> In C, please.

OK

Dmitry