linux-kernel - Re: [PATCH 4/4] uprobes/tracing: generalize struct uprobe_trace_entry

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]

Message-ID: <20130409145054.GA22913@redhat.com>
Date:	Tue, 9 Apr 2013 16:50:54 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Steven Rostedt <rostedt@...dmis.org>
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 4/4] uprobes/tracing: generalize struct
	uprobe_trace_entry_head

On 04/08, Steven Rostedt wrote:
>
> On Fri, 2013-03-29 at 19:15 +0100, Oleg Nesterov wrote:
> >
> > @@ -491,20 +502,19 @@ static int uprobe_trace_func(struct trace_uprobe *tu, struct pt_regs *regs)
> >  	struct uprobe_trace_entry_head *entry;
> >  	struct ring_buffer_event *event;
> >  	struct ring_buffer *buffer;
> > -	u8 *data;
> > +	void *data;
> >  	int size, i;
> >  	struct ftrace_event_call *call = &tu->call;
> >
> > -	size = sizeof(*entry) + tu->size;
> > -
> > +	size = SIZEOF_TRACE_ENTRY(1) + tu->size;
>
> That '1' is confusing. When I first looked at this code, I thought it
> was a bug as it should have been '0' (thinking of arrays). And then I
> realized that you want the entry *after* the element.
>
> Instead of '1' and I assume '2' for ret probes, how about defining an
> enum:
>
> enum {
> 	UPROBE_NORM	= 1,
> 	UPROBE_RET	= 2,
> };
>
> and then you can have;
>
> 	size = SIZEOF_TRACE_ENTRY(UPROBE_NORM);
>
> and later:
>
> 	size = SIZEOF_TRACE_ENTRY(UPROBE_RET);
>
> Same goes for DATAOF_TRACE_ENTRY().
>
> This would make it a lot easier to understand and review, and much less
> bug prone when we have to deal with two different types of
> uprobe_trace_entry_head's.

OK, will do.

Or. Instead of enum we can use "bool is_return". So, instead of

	if (is_ret_probe(tu))
		size = SIZEOF_TRACE_ENTRY(UPROBE_ENTRY_RETPROBE);
	else
		size = SIZEOF_TRACE_ENTRY(UPROBE_ENTRY_NORMAL);

we can do

	size = SIZEOF_TRACE_ENTRY(is_ret_probe(tu));

What do you like more?

Oleg.

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