Re: [PATCH 2/3] tracing: Extract out common code forkprobes/uprobes traceevents

[Steven Rostedt]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/