LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] tracing: silence GCC 9 array bounds warning
@ 2019-05-22  9:58 Miguel Ojeda
  2019-05-22 11:52 ` Steven Rostedt
  0 siblings, 1 reply; 4+ messages in thread
From: Miguel Ojeda @ 2019-05-22  9:58 UTC (permalink / raw)
  To: Steven Rostedt, Ingo Molnar; +Cc: Linus Torvalds, linux-kernel

Starting with GCC 9, -Warray-bounds detects cases when memset is called
starting on a member of a struct but the size to be cleared ends up
writing over further members.

Such a call happens in the trace code to clear, at once, all members
after and including `seq` on struct trace_iterator:

    In function 'memset',
        inlined from 'ftrace_dump' at kernel/trace/trace.c:8914:3:
    ./include/linux/string.h:344:9: warning: '__builtin_memset' offset
    [8505, 8560] from the object at 'iter' is out of the bounds of
    referenced subobject 'seq' with type 'struct trace_seq' at offset
    4368 [-Warray-bounds]
      344 |  return __builtin_memset(p, c, size);
          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

In order to avoid GCC complaining about it, we compute the address
ourselves by adding the offsetof distance instead of referring
directly to the member.

Since there are two places doing this clear (trace.c and trace_kdb.c),
take the chance to move the workaround into a single place in
the internal header.

Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
Steven, let me know if you still prefer (char *) or anything else
(or just change it in your side if you want :-)

 kernel/trace/trace.c     |  6 +-----
 kernel/trace/trace.h     | 14 ++++++++++++++
 kernel/trace/trace_kdb.c |  6 +-----
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 2c92b3d9ea30..1c80521fd436 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8910,12 +8910,8 @@ void ftrace_dump(enum ftrace_dump_mode oops_dump_mode)
 
 		cnt++;
 
-		/* reset all but tr, trace, and overruns */
-		memset(&iter.seq, 0,
-		       sizeof(struct trace_iterator) -
-		       offsetof(struct trace_iterator, seq));
+		trace_iterator_reset(&iter);
 		iter.iter_flags |= TRACE_FILE_LAT_FMT;
-		iter.pos = -1;
 
 		if (trace_find_next_entry_inc(&iter) != NULL) {
 			int ret;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 1974ce818ddb..f9a12003f137 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1967,4 +1967,18 @@ static inline void tracer_hardirqs_off(unsigned long a0, unsigned long a1) { }
 
 extern struct trace_iterator *tracepoint_print_iter;
 
+/* reset all but tr, trace, and overruns */
+static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
+{
+	/*
+	 * We do not simplify the start address to &iter->seq in order to let
+	 * GCC 9 know that we really want to overwrite more members than
+	 * just iter->seq (-Warray-bounds).
+	 */
+	const size_t offset = offsetof(struct trace_iterator, seq);
+	memset((char *)(iter) + offset, 0, sizeof(struct trace_iterator) - offset);
+
+	iter->pos = -1;
+}
+
 #endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c
index 6c1ae6b752d1..cca65044c14c 100644
--- a/kernel/trace/trace_kdb.c
+++ b/kernel/trace/trace_kdb.c
@@ -37,12 +37,8 @@ static void ftrace_dump_buf(int skip_entries, long cpu_file)
 	if (skip_entries)
 		kdb_printf("(skipping %d entries)\n", skip_entries);
 
-	/* reset all but tr, trace, and overruns */
-	memset(&iter.seq, 0,
-		   sizeof(struct trace_iterator) -
-		   offsetof(struct trace_iterator, seq));
+	trace_iterator_reset(&iter);
 	iter.iter_flags |= TRACE_FILE_LAT_FMT;
-	iter.pos = -1;
 
 	if (cpu_file == RING_BUFFER_ALL_CPUS) {
 		for_each_tracing_cpu(cpu) {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] tracing: silence GCC 9 array bounds warning
  2019-05-22  9:58 [PATCH v2] tracing: silence GCC 9 array bounds warning Miguel Ojeda
@ 2019-05-22 11:52 ` Steven Rostedt
  2019-05-22 13:11   ` Miguel Ojeda
  0 siblings, 1 reply; 4+ messages in thread
From: Steven Rostedt @ 2019-05-22 11:52 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: Ingo Molnar, Linus Torvalds, linux-kernel

On Wed, 22 May 2019 11:58:10 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> +/* reset all but tr, trace, and overruns */
> +static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
> +{
> +	/*
> +	 * We do not simplify the start address to &iter->seq in order to let
> +	 * GCC 9 know that we really want to overwrite more members than
> +	 * just iter->seq (-Warray-bounds).

This comment is fine for the change log, but here it is too specific.
Why does one care about GCC 9 when we are at version GCC 21? I care
more about why we are clearing the data and less about the way we are
doing it.

A comment like:

	/*
	 * Reset the state of the trace_iterator so that it can read
	 * consumed data. Normally, the trace_iterator is used for
	 * reading the data when it is not consumed, and must retain
	 * state.
	 */

That is more useful than why we have the offset hack.


> +	 */
> +	const size_t offset = offsetof(struct trace_iterator, seq);

Need a empty line between these two.

-- Steve

> +	memset((char *)(iter) + offset, 0, sizeof(struct trace_iterator) - offset);
> +
> +	iter->pos = -1;
> +}
> +
>  #endif /* _LINUX_KERNEL_TRACE_H */

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] tracing: silence GCC 9 array bounds warning
  2019-05-22 11:52 ` Steven Rostedt
@ 2019-05-22 13:11   ` Miguel Ojeda
  2019-05-22 13:34     ` Steven Rostedt
  0 siblings, 1 reply; 4+ messages in thread
From: Miguel Ojeda @ 2019-05-22 13:11 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Ingo Molnar, Linus Torvalds, linux-kernel

On Wed, May 22, 2019 at 1:52 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 22 May 2019 11:58:10 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > +/* reset all but tr, trace, and overruns */
> > +static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
> > +{
> > +     /*
> > +      * We do not simplify the start address to &iter->seq in order to let
> > +      * GCC 9 know that we really want to overwrite more members than
> > +      * just iter->seq (-Warray-bounds).
>
> This comment is fine for the change log, but here it is too specific.
> Why does one care about GCC 9 when we are at version GCC 21? I care
> more about why we are clearing the data and less about the way we are
> doing it.

Since the code is not written the obvious way on purpose, the idea is
to document why that is so -- otherwise the reader may wonder (and
possibly re-introduce it back). Specifying when the warning started
appearing tends to be clarifying, too.

The commit message explains the change itself, but the comment
explains why the current code is written like that.

> A comment like:
>
>         /*
>          * Reset the state of the trace_iterator so that it can read
>          * consumed data. Normally, the trace_iterator is used for
>          * reading the data when it is not consumed, and must retain
>          * state.
>          */
>
> That is more useful than why we have the offset hack.

That comment would be great in the function's description, in my
opinion, and it is a great addition to have nevertheless. I re-used
the existing comment for that to keep the change as minimal as
possible (and nevertheless I am not qualified to write it since I have
not studied the tracing code). In other words, I'm not saying there
are no further improvements :-)

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] tracing: silence GCC 9 array bounds warning
  2019-05-22 13:11   ` Miguel Ojeda
@ 2019-05-22 13:34     ` Steven Rostedt
  0 siblings, 0 replies; 4+ messages in thread
From: Steven Rostedt @ 2019-05-22 13:34 UTC (permalink / raw)
  To: Miguel Ojeda; +Cc: Ingo Molnar, Linus Torvalds, linux-kernel

On Wed, 22 May 2019 15:11:10 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Wed, May 22, 2019 at 1:52 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Wed, 22 May 2019 11:58:10 +0200
> > Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> >  
> > > +/* reset all but tr, trace, and overruns */
> > > +static __always_inline void trace_iterator_reset(struct trace_iterator *iter)
> > > +{
> > > +     /*
> > > +      * We do not simplify the start address to &iter->seq in order to let
> > > +      * GCC 9 know that we really want to overwrite more members than
> > > +      * just iter->seq (-Warray-bounds).  
> >
> > This comment is fine for the change log, but here it is too specific.
> > Why does one care about GCC 9 when we are at version GCC 21? I care
> > more about why we are clearing the data and less about the way we are
> > doing it.  
> 
> Since the code is not written the obvious way on purpose, the idea is
> to document why that is so -- otherwise the reader may wonder (and
> possibly re-introduce it back). Specifying when the warning started
> appearing tends to be clarifying, too.
> 
> The commit message explains the change itself, but the comment
> explains why the current code is written like that.

Could also be shorten to: "Keep gcc from complaining about overwriting
more than just one member in the structure."


> 
> > A comment like:
> >
> >         /*
> >          * Reset the state of the trace_iterator so that it can read
> >          * consumed data. Normally, the trace_iterator is used for
> >          * reading the data when it is not consumed, and must retain
> >          * state.
> >          */
> >
> > That is more useful than why we have the offset hack.  
> 
> That comment would be great in the function's description, in my
> opinion, and it is a great addition to have nevertheless. I re-used
> the existing comment for that to keep the change as minimal as
> possible (and nevertheless I am not qualified to write it since I have
> not studied the tracing code). In other words, I'm not saying there
> are no further improvements :-)

I put it there so you have an idea what the rational was ;-)

-- Steve

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-05-22 13:34 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-22  9:58 [PATCH v2] tracing: silence GCC 9 array bounds warning Miguel Ojeda
2019-05-22 11:52 ` Steven Rostedt
2019-05-22 13:11   ` Miguel Ojeda
2019-05-22 13:34     ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).