linux-kernel - Re: [PATCH 3/4] uprobes/tracing: Kill the pointless local_save_flags/preempt

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 08 Apr 2013 11:58:42 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Anton Arapov <anton@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] uprobes/tracing: Kill the pointless
 local_save_flags/preempt_count calls

On Fri, 2013-03-29 at 19:15 +0100, Oleg Nesterov wrote:
> uprobe_trace_func() is never called with irqs or preemption
> disabled, no need to ask preempt_count() or local_save_flags().
> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  kernel/trace/trace_uprobe.c |   10 +++-------
>  1 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> index 8e00901..43d258d 100644
> --- a/kernel/trace/trace_uprobe.c
> +++ b/kernel/trace/trace_uprobe.c
> @@ -492,17 +492,13 @@ static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  	struct ring_buffer_event *event;
>  	struct ring_buffer *buffer;
>  	u8 *data;
> -	int size, i, pc;
> -	unsigned long irq_flags;
> +	int size, i;
>  	struct ftrace_event_call *call = &tu->call;
>  
> -	local_save_flags(irq_flags);
> -	pc = preempt_count();

How about instead, just change the above two and have:

	/* uprobes are never called with preemption disabled */
	pc = 0;
	irq_flags = 0;

and leave the rest the same. This will help in future reviewers of the
code to not have to look up what that "0, 0" is for, and then wonder if
it should be that way. gcc should optimize it to be exactly the same as
this patch.

-- Steve

> -
>  	size = sizeof(*entry) + tu->size;
>  
>  	event = trace_current_buffer_lock_reserve(&buffer, call->event.type,
> -						  size, irq_flags, pc);
> +						  size, 0, 0);
>  	if (!event)
>  		return 0;
>  
> @@ -513,7 +509,7 @@ static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
>  		call_fetch(&tu->args[i].fetch, regs, data + tu->args[i].offset);
>  
>  	if (!filter_current_check_discard(buffer, call, entry, event))
> -		trace_buffer_unlock_commit(buffer, event, irq_flags, pc);
> +		trace_buffer_unlock_commit(buffer, event, 0, 0);
>  
>  	return 0;
>  }


--
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/