| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1357941855.5141.43.camel@gandalf.local.home>
Date: Fri, 11 Jan 2013 17:04:15 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Keun-O Park <kpark3469@...il.com>
Cc: Russell King - ARM Linux <linux@....linux.org.uk>,
fweisbec@...il.com, mingo@...hat.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
sahara <keun-o.park@...driver.com>
Subject: Re: [PATCH 2/2] arm: make return_address available for ARM_UNWIND
On Fri, 2013-01-11 at 17:53 +0900, Keun-O Park wrote:
> Hello Steve,
>
> Much obliged for your explaining this.
> As your guess, there's another path that uses CALLER_ADDR1 without
> CONFIG_PROVE_LOCKING though both lockdep and ftrace's irqsoff tracer
> is turned on. In this case, ftrace's trace_hardirqs_on/off() would be
> called.
> And, as you said to me, if lockdep is off and ftrace's irqsoff is on,
> ftrace's trace_hardirqs_on/off() would be called.
> I think the proper way to avoid calling CALLER_ADDR1 will be calling
> trace_hardirqs_off_caller() instead of calling stop_critical_timing()
> directly in trace_hardirqs_off(). And, this will suppress the message
> outputs of unwind_frame.
That will break the output for users that have a working CALLER_ADDR1.
> If you think this idea is okay, I will push the patch v2 to you.
> In my test result, it looks it's functionally correct.
>
> Thanks.
>
> -- kpark
>
>
> diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
> index f89515a..3552ad9 100644
> --- a/arch/arm/include/asm/ftrace.h
> +++ b/arch/arm/include/asm/ftrace.h
> @@ -32,13 +32,11 @@ extern void ftrace_call_old(void);
>
> #ifndef __ASSEMBLY__
>
> -#if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
> +#if defined(CONFIG_FRAME_POINTER) || defined(CONFIG_ARM_UNWIND)
> /*
> * return_address uses walk_stackframe to do it's work. If both
> * CONFIG_FRAME_POINTER=y and CONFIG_ARM_UNWIND=y walk_stackframe uses unwind
> - * information. For this to work in the function tracer many functions would
> - * have to be marked with __notrace. So for now just depend on
> - * !CONFIG_ARM_UNWIND.
> + * information.
> */
>
> void *return_address(unsigned int);
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 5bbec7b..675fd62 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -9,6 +9,9 @@ ifdef CONFIG_FUNCTION_TRACER
> CFLAGS_REMOVE_ftrace.o = -pg
> CFLAGS_REMOVE_insn.o = -pg
> CFLAGS_REMOVE_patch.o = -pg
> +ifdef CONFIG_ARM_UNWIND
> +CFLAGS_REMOVE_unwind.o = -pg
> +endif
You don't need to add the ifdef CONFIG_ARM_UNWIND. It's just setting a
variable and if the unwind.o gets complied, it will apply the removal of
-pg. If the unwind isn't compiled the variable will just be set but
unused. No harm done.
> endif
>
> CFLAGS_REMOVE_return_address.o = -pg
> --- a/kernel/trace/trace_irqsoff.c
> +++ b/kernel/trace/trace_irqsoff.c
> @@ -483,20 +483,6 @@ inline void print_irqtrace_events(struct task_struct *curr)
> /*
> * We are only interested in hardirq on/off events:
> */
> -void trace_hardirqs_on(void)
> -{
> - if (!preempt_trace() && irq_trace())
> - stop_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
> -}
> -EXPORT_SYMBOL(trace_hardirqs_on);
> -
> -void trace_hardirqs_off(void)
> -{
> - if (!preempt_trace() && irq_trace())
> - start_critical_timing(CALLER_ADDR0, CALLER_ADDR1);
> -}
> -EXPORT_SYMBOL(trace_hardirqs_off);
> -
> void trace_hardirqs_on_caller(unsigned long caller_addr)
> {
> if (!preempt_trace() && irq_trace())
> @@ -504,6 +490,12 @@ void trace_hardirqs_on_caller(unsigned long caller_addr)
> }
> EXPORT_SYMBOL(trace_hardirqs_on_caller);
>
> +void trace_hardirqs_on(void)
> +{
> + trace_hardirqs_on_caller(CALLER_ADDR0);
> +}
> +EXPORT_SYMBOL(trace_hardirqs_on);
This is not equivalent. CALLER_ADDR0 will just give us what disabled the
irqs (spin_lock_irq or local_irq_save), but wont give us who called
that.
If you can't handle CALLER_ADDR1, then define it to NULL or to
CALLER_ADDR0.
What the original output gives:
jbd2/dm--1399 3d... 0us : _raw_spin_lock_irqsave
<-ata_scsi_queuecmd
which shows that the _raw_spin_lock_irqsave which disabled irqs was
called by ata_scsi_queuecmd.
With your patch we get:
jbd2/dm--1407 3d... 0us : trace_hardirqs_off
<-_raw_spin_lock_irqsave
Which shows the internal function "trace_hardirqs_off" which is useless,
as well as that the thing that disabled interrupts was
_raw_spin_lock_irqsave. But we don't get who called
_raw_spin_lock_irqsave.
Thus you just broke the code for those that can handle CALLER_ADDR1.
-- Steve
> +
> void trace_hardirqs_off_caller(unsigned long caller_addr)
> {
> if (!preempt_trace() && irq_trace())
> @@ -511,6 +503,12 @@ void trace_hardirqs_off_caller(unsigned long caller_addr)
> }
> EXPORT_SYMBOL(trace_hardirqs_off_caller);
>
> +void trace_hardirqs_off(void)
> +{
> + trace_hardirqs_off_caller(CALLER_ADDR0);
> +}
> +EXPORT_SYMBOL(trace_hardirqs_off);
> +
> #endif /* CONFIG_PROVE_LOCKING */
> #endif /* CONFIG_IRQSOFF_TRACER */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists