Re: [PATCH v3 5/5] tracing/probe: Support user-space dereference

From: Masami Hiramatsu <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org,
	Andy Lutomirski <luto@amacapital.net>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Changbin Du <changbin.du@gmail.com>, Jann Horn <jannh@google.com>,
	Kees Cook <keescook@chromium.org>,
	Andy Lutomirski <luto@kernel.org>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Nadav Amit <namit@vmware.com>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v3 5/5] tracing/probe: Support user-space dereference
Date: Thu, 28 Feb 2019 15:08:34 +0900	[thread overview]
Message-ID: <20190228150834.347a778e4360e91fcc6d5082@kernel.org> (raw)
In-Reply-To: <20190227214245.39d6563f@vmware.local.home>

On Wed, 27 Feb 2019 21:42:45 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 27 Feb 2019 23:44:42 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index a7012de37a00..0efef172db17 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -239,6 +239,7 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
> >  {
> >  	struct fetch_insn *code = *pcode;
> >  	unsigned long param;
> > +	int deref = FETCH_OP_DEREF;
> >  	long offset = 0;
> >  	char *tmp;
> >  	int ret = 0;
> > @@ -301,8 +302,17 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
> >  		break;
> >  
> >  	case '+':	/* deref memory */
> > -		arg++;	/* Skip '+', because kstrtol() rejects it. */
> >  	case '-':
> > +		if (arg[0] == '+') {
> > +			arg++;	/* Skip '+', because kstrtol() rejects it. */
> > +			if (arg[0] == 'u') {
> > +				deref = FETCH_OP_UDEREF;
> > +				arg++;
> > +			}
> > +		} else if (arg[1] == 'u') {	/* Start with "-u" */
> > +			deref = FETCH_OP_UDEREF;
> > +			*(++arg) = '-';
> > +		}
> 
> What about:
> 
> 		if (arg[1] == 'u') {
> 			deref = FETCH_OP_UDEREF;
> 			arg[1] = arg[0];
> 			arg++;
> 		}
> 		if (arg[0] == '+')
> 			arg++; /* Skip '+', because kstrtol() rejects it. */
> 
> A bit less messy.

Ah, thanks! I'll take it.

> 
> 
> >  		tmp = strchr(arg, '(');
> >  		if (!tmp)
> >  			return -EINVAL;
> > @@ -328,7 +338,7 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
> >  				return -E2BIG;
> >  			*pcode = code;
> >  
> > -			code->op = FETCH_OP_DEREF;
> > +			code->op = deref;
> >  			code->offset = offset;
> >  		}
> >  		break;
> > @@ -444,13 +454,14 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
> >  	/* Store operation */
> >  	if (!strcmp(parg->type->name, "string") ||
> >  	    !strcmp(parg->type->name, "ustring")) {
> > -		if (code->op != FETCH_OP_DEREF && code->op != FETCH_OP_IMM &&
> > -		    code->op != FETCH_OP_COMM) {
> > +		if (code->op != FETCH_OP_DEREF && code->op != FETCH_OP_UDEREF
> > +		    && code->op != FETCH_OP_IMM && code->op != FETCH_OP_COMM) {
> >  			pr_info("string only accepts memory or address.\n");
> >  			ret = -EINVAL;
> >  			goto fail;
> >  		}
> > -		if (code->op != FETCH_OP_DEREF || parg->count) {
> > +		if ((code->op == FETCH_OP_IMM && code->op == FETCH_OP_COMM)
> > +		    || parg->count) {
> 
> How would "code->op == FETCH_OP_IMM && code->op == FETCH_OP_COMM" ever be true?
> 
> Did you mean || ?

Oops, yes. It is a simple mistake...

Thank you!

> 
> -- Steve
> 
> >  			/*
> >  			 * IMM and COMM is pointing actual address, those must
> >  			 * be kept, and if parg->count != 0, this is an array
> > @@ -463,7 +474,8 @@ static int traceprobe_parse_probe_arg_body(char *arg, ssize_t *size,
> >  			}
> >  		}
> >  		/* If op == DEREF, replace it with STRING */
> > -		if (!strcmp(parg->type->name, "ustring"))
> > +		if (!strcmp(parg->type->name, "ustring") ||
> > +		    code->op == FETCH_OP_UDEREF)
> >  			code->op = FETCH_OP_ST_USTRING;
> >  		else
> >  			code->op = FETCH_OP_ST_STRING;

-- 
Masami Hiramatsu <mhiramat@kernel.org>