linux-kernel - Re: [PATCH 2/3] tracing: Extract out common code for kprobes/uprobes traceevents

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1333644941.3764.32.camel@pippen.local.home>
Date:	Thu, 05 Apr 2012 12:55:41 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Jim Keniston <jkenisto@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux-mm <linux-mm@...ck.org>, Oleg Nesterov <oleg@...hat.com>,
	Andi Kleen <andi@...stfloor.org>,
	Christoph Hellwig <hch@...radead.org>,
	Arnaldo Carvalho de Melo <acme@...radead.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Anton Arapov <anton@...hat.com>
Subject: Re: [PATCH 2/3] tracing: Extract out common code for
 kprobes/uprobes traceevents

On Tue, 2012-04-03 at 06:34 +0530, Srikar Dronamraju wrote:


> -/*
> - * Fetch a null-terminated string. Caller MUST set *(u32 *)dest with max
> - * length and relative data location.
> - */
> -static __kprobes void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
> -                                                     void *addr, void *dest)
> -{
> -       long ret;
> -       int maxlen = get_rloc_len(*(u32 *)dest);
> -       u8 *dst = get_rloc_data(dest);
> -       u8 *src = addr;
> -       mm_segment_t old_fs = get_fs();
> -       if (!maxlen)
> -               return;

> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> new file mode 100644
> index 0000000..deb375a
> --- /dev/null
> +++ b/kernel/trace/trace_probe.c

> +DEFINE_BASIC_FETCH_FUNCS(memory)
> +/*
> + * Fetch a null-terminated string. Caller MUST set *(u32 *)dest with max
> + * length and relative data location.
> + */
> +static __kprobes void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
> +						      void *addr, void *dest)
> +{
> +	int maxlen;
> +	long ret;
> +
> +	maxlen = get_rloc_len(*(u32 *)dest);
> +	u8 *dst = get_rloc_data(dest);
> +	u8 *src = addr;


Please do not mix declarations and actual code. The above declares
maxlen and ret, then assigns maxlen (actual code) and then you declare
dst and src (as well as assign it).

The original version (shown at the top) is fine. Why did you change it?
Although the original should have a space:

static __kprobes void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs,
                                                     void *addr, void *dest)
{
       long ret;
       int maxlen = get_rloc_len(*(u32 *)dest);
       u8 *dst = get_rloc_data(dest);
       u8 *src = addr;
       mm_segment_t old_fs = get_fs();
					<--- new line needed (from original)
       if (!maxlen)
               return;

> +	mm_segment_t old_fs = get_fs();
> +
> +	if (!maxlen)
> +		return;
> +
> +	/*
> +	 * Try to get string again, since the string can be changed while
> +	 * probing.
> +	 */
> +	set_fs(KERNEL_DS);
> +	pagefault_disable();
> +
> +	do
> +		ret = __copy_from_user_inatomic(dst++, src++, 1);
> +	while (dst[-1] && ret == 0 && src - (u8 *)addr < maxlen);
> +
> +	dst[-1] = '\0';
> +	pagefault_enable();
> +	set_fs(old_fs);
> +
> +	if (ret < 0) {	/* Failed to fetch string */
> +		((u8 *)get_rloc_data(dest))[0] = '\0';
> +		*(u32 *)dest = make_data_rloc(0, get_rloc_offs(*(u32 *)dest));
> +	} else {
> +		*(u32 *)dest = make_data_rloc(src - (u8 *)addr,
> +					      get_rloc_offs(*(u32 *)dest));
> +	}
> +}


> +
> +#define WRITE_BUFSIZE 128

The original code had WRITE_BUFSIZE as 4096 this has it with 128. That's
a big difference. Even if you have a reason for changing this, don't do
it in this patch. That should be a separate patch with an explanation of
why this was changed.

The rest of the patch looks fine.

-- Steve

> +
> +ssize_t traceprobe_probes_write(struct file *file, const char __user *buffer,
> +				size_t count, loff_t *ppos,
> +				int (*createfn)(int, char **))
> +{
> +	char *kbuf, *tmp;
> +	int ret = 0;
> +	size_t done = 0;
> +	size_t size;
> +
> +
> 


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