Re: [PATCH v7 3/6] tracing/probe: Add ustring type for user-space string

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:

> Add "ustring" type for fetching user-space string from kprobe event.
> User can specify ustring type at uprobe event, and it is same as
> "string" for uprobe.
> 
> Note that probe-event provides this option but it doesn't choose the
> correct type automatically since we have not way to decide the address
> is in user-space or not on some arch (and on some other arch, you can
> fetch the string by "string" type). So user must carefully check the
> target code (e.g. if you see __user on the target variable) and
> use this new type.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Acked-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
> 
> ---
>  Changes in v5:
>  - Use strnlen_unsafe_user() in fetch_store_strlen_user().
>  Changes in v2:
>  - Use strnlen_user() instead of open code for fetch_store_strlen_user().
> ---
>  Documentation/trace/kprobetrace.rst |    9 +++++++--
>  kernel/trace/trace.c                |    2 +-
>  kernel/trace/trace_kprobe.c         |   29 +++++++++++++++++++++++++++++
>  kernel/trace/trace_probe.c          |   14 +++++++++++---
>  kernel/trace/trace_probe.h          |    1 +
>  kernel/trace/trace_probe_tmpl.h     |   14 +++++++++++++-
>  kernel/trace/trace_uprobe.c         |   12 ++++++++++++
>  7 files changed, 74 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/trace/kprobetrace.rst b/Documentation/trace/kprobetrace.rst
> index 235ce2ab131a..a3ac7c9ac242 100644
> --- a/Documentation/trace/kprobetrace.rst
> +++ b/Documentation/trace/kprobetrace.rst
> @@ -55,7 +55,8 @@ Synopsis of kprobe_events
>    NAME=FETCHARG : Set NAME as the argument name of FETCHARG.
>    FETCHARG:TYPE : Set TYPE as the type of FETCHARG. Currently, basic types
>  		  (u8/u16/u32/u64/s8/s16/s32/s64), hexadecimal types
> -		  (x8/x16/x32/x64), "string" and bitfield are supported.
> +		  (x8/x16/x32/x64), "string", "ustring" and bitfield
> +		  are supported.
>  
>    (\*1) only for the probe on function entry (offs == 0).
>    (\*2) only for return probe.
> @@ -77,7 +78,11 @@ apply it to registers/stack-entries etc. (for example, '$stack1:x8[8]' is
>  wrong, but '+8($stack):x8[8]' is OK.)
>  String type is a special type, which fetches a "null-terminated" string from
>  kernel space. This means it will fail and store NULL if the string container
> -has been paged out.
> +has been paged out. "ustring" type is an alternative of string for user-space.
> +Note that kprobe-event provides string/ustring types, but doesn't change it
> +automatically. So user has to decide if the targe string in kernel or in user
> +space carefully. On some arch, if you choose wrong one, it always fails to
> +record string data.
>  The string array type is a bit different from other types. For other base
>  types, <base-type>[1] is equal to <base-type> (e.g. +0(%di):x32[1] is same
>  as +0(%di):x32.) But string[1] is not equal to string. The string type itself
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index dcb9adb44be9..101a5d09a632 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4858,7 +4858,7 @@ static const char readme_msg[] =
>  	"\t           $stack<index>, $stack, $retval, $comm\n"
>  #endif
>  	"\t     type: s8/16/32/64, u8/16/32/64, x8/16/32/64, string, symbol,\n"
> -	"\t           b<bit-width>@<bit-offset>/<container-size>,\n"
> +	"\t           b<bit-width>@<bit-offset>/<container-size>, ustring,\n"
>  	"\t           <type>\\[<array-size>\\]\n"
>  #ifdef CONFIG_HIST_TRIGGERS
>  	"\t    field: <stype> <name>;\n"
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 7d736248a070..fcb8806fc93c 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -886,6 +886,14 @@ fetch_store_strlen(unsigned long addr)
>  	return (ret < 0) ? ret : len;
>  }
>  
> +/* Return the length of string -- including null terminal byte */
> +static nokprobe_inline int
> +fetch_store_strlen_user(unsigned long addr)
> +{
> +	return strnlen_unsafe_user((__force const void __user *)addr,
> +				   MAX_STRING_SIZE);
> +}
> +
>  /*
>   * Fetch a null-terminated string. Caller MUST set *(u32 *)buf with max
>   * length and relative data location.
> @@ -910,6 +918,27 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
>  	return ret;
>  }
>  
> +/*
> + * Fetch a null-terminated string from user. Caller MUST set *(u32 *)buf
> + * with max length and relative data location.
> + */
> +static nokprobe_inline int
> +fetch_store_string_user(unsigned long addr, void *dest, void *base)
> +{
> +	int maxlen = get_loc_len(*(u32 *)dest);
> +	u8 *dst = get_loc_data(dest, base);
> +	long ret;
> +
> +	if (unlikely(!maxlen))
> +		return -ENOMEM;
> +	ret = strncpy_from_unsafe_user(dst, (__force const void __user *)addr,
> +				       maxlen);

Pointless line break.

> +
> +	if (ret >= 0)
> +		*(u32 *)dest = make_data_loc(ret, (void *)dst - base);
> +	return ret;
> +}

Plus the problem as in the earlier patch.

Thanks,

	Ingo