Re: [PATCH -mm] tracing: Fix section mismatch intrace_hw_branches.c

[Frederic Weisbecker]

On Sat, Feb 14, 2009 at 09:36:00AM +0600, Rakib Mullick wrote:
>   Impact: Fix section mismatch
> 
> The function bts_trace_init() references a variable
> bts_hotcpu_notifier which is marked
> as __cpuinitdata. Thus causes section mismatch. This patch fixes it.
> 
>   LD      kernel/trace/built-in.o
> WARNING: kernel/trace/built-in.o(.text+0xc90c): Section mismatch in
> reference from the function bts_trace_init() to the variable
> .cpuinit.data:bts_hotcpu_notifier
> The function bts_trace_init() references
> the variable __cpuinitdata bts_hotcpu_notifier.
> This is often because bts_trace_init lacks a __cpuinitdata
> annotation or the annotation of bts_hotcpu_notifier is wrong.
> 
> WARNING: kernel/trace/built-in.o(.text+0xc92a): Section mismatch in
> reference from the function bts_trace_reset() to the variable
> .cpuinit.data:bts_hotcpu_notifier
> The function bts_trace_reset() references
> the variable __cpuinitdata bts_hotcpu_notifier.
> This is often because bts_trace_reset lacks a __cpuinitdata
> annotation or the annotation of bts_hotcpu_notifier is wrong.
> 
> Thanks.
> 
> ---
> Signed-off-by: Rakib Mullick <rakib.mullick@xxxxxxxxx>
> 
> --- linus/kernel/trace/trace_hw_branches.c	2009-02-13 11:23:55.000000000 +0600
> +++ rakib/kernel/trace/trace_hw_branches.c	2009-02-13 22:12:30.000000000 +0600
> @@ -127,7 +127,7 @@ static struct notifier_block bts_hotcpu_
>  	.notifier_call = bts_hotcpu_handler
>  };
> 
> -static int bts_trace_init(struct trace_array *tr)
> +static int __cpuinit bts_trace_init(struct trace_array *tr)
>  {
>  	hw_branch_trace = tr;
> 
> @@ -137,7 +137,7 @@ static int bts_trace_init(struct trace_a
>  	return 0;
>  }
> 
> -static void bts_trace_reset(struct trace_array *tr)
> +static void __cpuinit bts_trace_reset(struct trace_array *tr)
>  {
>  	bts_trace_stop(tr);
>  	unregister_hotcpu_notifier(&bts_hotcpu_notifier);


Hi,

When I saw this patch, I searched the real purpose of __cpuinit and its
real impact.
But I didn't find any comments about it inside the kernel.

But today, by looking at the discussion around latest git pull for x86
to mainline, I discover that __cpuinit becomes __init on UP.

So, unless I missed something, this patch seems to me very dangerous.
The init and reset callbacks of a tracer can be called at any time, not only
on initcalls time (__init functions are freed from memory after the middle stage
of the boot).
With this patch, on UP we will dereference freed memory while activating this tracer.

The old code was fine because register_hotplug_cpu does nothing on UP.
Unfortunately the warning still existed though this was a kind of false positive.
This is a section mismatch, but harmless.

So instead I would suggest to:

- call register_hotcpu_notifier(&bts_hotcpu_notifier) from init_bts_trace() which
  is called only one time on boot.

- never unregister this notifier

- inside bts_hotcpu_handler(), only call bts_trace_{start,stop}_cpu() on the given
  cpu if trace_hw_branches_enabled == 1
  Ok, now the handler will be called on each cpu hotplug event but this is fine since
  this is a rare path.

Hm?


> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/