diff mbox series

[v3,4/4] tracing/probes: Fix to record 0-length data_loc in fetch_store_string*() if fails

Message ID 168878453829.2721251.15110493517953858343.stgit@mhiramat.roam.corp.google.com (mailing list archive)
State Superseded
Delegated to: Masami Hiramatsu
Headers show
Series tracing/probes: Fix bugs in process_fetch_insn | expand

Commit Message

Masami Hiramatsu (Google) July 8, 2023, 2:48 a.m. UTC 
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Fix to record 0-length data to data_loc in fetch_store_string*() if it fails
to get the string data.
Currently those expect that the data_loc is updated by store_trace_args() if
it returns the error code. However, that does not work correctly if the
argument is an array of strings. In that case, store_trace_args() only clears
the first entry of the array (which may have no error) and leaves other
entries. So it should be cleared by fetch_store_string*() itself.
Also, 'dyndata' and 'maxlen' in store_trace_args() should be updated
only if it is used (ret > 0 and argument is a dynamic data.)

Fixes: 40b53b771806 ("tracing: probeevent: Add array type support")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace_probe_kernel.h |    6 ++----
 kernel/trace/trace_probe_tmpl.h   |    4 +---
 kernel/trace/trace_uprobe.c       |    3 ++-
 3 files changed, 5 insertions(+), 8 deletions(-)

Comments

Steven Rostedt July 10, 2023, 10:16 p.m. UTC | #1 
On Sat,  8 Jul 2023 11:48:58 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> --- a/kernel/trace/trace_probe_kernel.h
> +++ b/kernel/trace/trace_probe_kernel.h
> @@ -55,8 +55,7 @@ fetch_store_string_user(unsigned long addr, void *dest, void *base)
>  	__dest = get_loc_data(dest, base);
>  
>  	ret = strncpy_from_user_nofault(__dest, uaddr, maxlen);
> -	if (ret >= 0)
> -		*(u32 *)dest = make_data_loc(ret, __dest - base);
> +	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);
>  
>  	return ret;
>  }
> @@ -87,8 +86,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
>  	 * probing.
>  	 */
>  	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
> -	if (ret >= 0)
> -		*(u32 *)dest = make_data_loc(ret, __dest - base);
> +	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);

The above is a complex line, and not something that I think should be cut
and pasted between two different locations.

I know you took out the set_data_loc() helper, but really it should have
stayed, and have used that to update this code in the two places it
affected, instead of making the changes in those two locations.

That is, patch 3 could have had kept.

static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
{
	if (ret >= 0)
		*(u32 *)dest = make_data_loc(ret, __dest - base);
}

And this patch could have been:

static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
{
	*(u32 *)dest = make_data_loc(ret, __dest - base);
}

That would keep the complexity down in this changes set.

-- Steve


>  
>  	return ret;
>  }
Masami Hiramatsu (Google) July 11, 2023, 12:05 a.m. UTC | #2 
On Mon, 10 Jul 2023 18:16:01 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat,  8 Jul 2023 11:48:58 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > --- a/kernel/trace/trace_probe_kernel.h
> > +++ b/kernel/trace/trace_probe_kernel.h
> > @@ -55,8 +55,7 @@ fetch_store_string_user(unsigned long addr, void *dest, void *base)
> >  	__dest = get_loc_data(dest, base);
> >  
> >  	ret = strncpy_from_user_nofault(__dest, uaddr, maxlen);
> > -	if (ret >= 0)
> > -		*(u32 *)dest = make_data_loc(ret, __dest - base);
> > +	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);
> >  
> >  	return ret;
> >  }
> > @@ -87,8 +86,7 @@ fetch_store_string(unsigned long addr, void *dest, void *base)
> >  	 * probing.
> >  	 */
> >  	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
> > -	if (ret >= 0)
> > -		*(u32 *)dest = make_data_loc(ret, __dest - base);
> > +	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);
> 
> The above is a complex line, and not something that I think should be cut
> and pasted between two different locations.
> 
> I know you took out the set_data_loc() helper, but really it should have
> stayed, and have used that to update this code in the two places it
> affected, instead of making the changes in those two locations.
> 
> That is, patch 3 could have had kept.
> 
> static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
> {
> 	if (ret >= 0)
> 		*(u32 *)dest = make_data_loc(ret, __dest - base);
> }

To avoid confusion, I would like to revert the set_data_loc() at the 3rd patch
and add it again in 4th patch.

> 
> And this patch could have been:
> 
> static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
> {
> 	*(u32 *)dest = make_data_loc(ret, __dest - base);
> }

and introduce it. I also want to put the ternary operator into set_data_loc() too
for simplicity.

static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
{
	if (ret < 0)
		ret = 0;
	*(u32 *)dest = make_data_loc(ret, __dest - base);
}

Thanks,

> 
> That would keep the complexity down in this changes set.
> 
> -- Steve
> 
> 
> >  
> >  	return ret;
> >  }
Steven Rostedt July 11, 2023, 12:16 a.m. UTC | #3 
On Tue, 11 Jul 2023 09:05:15 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > And this patch could have been:
> > 
> > static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
> > {
> > 	*(u32 *)dest = make_data_loc(ret, __dest - base);
> > }  
> 
> and introduce it. I also want to put the ternary operator into set_data_loc() too
> for simplicity.
> 
> static nokprobe_inline void set_data_loc(int ret, void *dest, void *__dest, void *base)
> {
> 	if (ret < 0)
> 		ret = 0;
> 	*(u32 *)dest = make_data_loc(ret, __dest - base);
> }
> 

Sounds good.

-- Steve
diff mbox series

Patch

diff --git a/kernel/trace/trace_probe_kernel.h b/kernel/trace/trace_probe_kernel.h
index 6deae2ce34f8..37d5696ed768 100644
--- a/kernel/trace/trace_probe_kernel.h
+++ b/kernel/trace/trace_probe_kernel.h
@@ -55,8 +55,7 @@  fetch_store_string_user(unsigned long addr, void *dest, void *base)
 	__dest = get_loc_data(dest, base);
 
 	ret = strncpy_from_user_nofault(__dest, uaddr, maxlen);
-	if (ret >= 0)
-		*(u32 *)dest = make_data_loc(ret, __dest - base);
+	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);
 
 	return ret;
 }
@@ -87,8 +86,7 @@  fetch_store_string(unsigned long addr, void *dest, void *base)
 	 * probing.
 	 */
 	ret = strncpy_from_kernel_nofault(__dest, (void *)addr, maxlen);
-	if (ret >= 0)
-		*(u32 *)dest = make_data_loc(ret, __dest - base);
+	*(u32 *)dest = make_data_loc((ret >= 0) ? ret : 0, __dest - base);
 
 	return ret;
 }
diff --git a/kernel/trace/trace_probe_tmpl.h b/kernel/trace/trace_probe_tmpl.h
index ed9d57c6b041..bbad0503f166 100644
--- a/kernel/trace/trace_probe_tmpl.h
+++ b/kernel/trace/trace_probe_tmpl.h
@@ -267,9 +267,7 @@  store_trace_args(void *data, struct trace_probe *tp, void *rec,
 		if (unlikely(arg->dynamic))
 			*dl = make_data_loc(maxlen, dyndata - base);
 		ret = process_fetch_insn(arg->code, rec, dl, base);
-		if (unlikely(ret < 0 && arg->dynamic)) {
-			*dl = make_data_loc(0, dyndata - base);
-		} else {
+		if (unlikely(ret > 0 && arg->dynamic)) {
 			dyndata += ret;
 			maxlen -= ret;
 		}
diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 8b92e34ff0c8..7b47e9a2c010 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -170,7 +170,8 @@  fetch_store_string(unsigned long addr, void *dest, void *base)
 			 */
 			ret++;
 		*(u32 *)dest = make_data_loc(ret, (void *)dst - base);
-	}
+	} else
+		*(u32 *)dest = make_data_loc(0, (void *)dst - base);
 
 	return ret;
 }