On Wed, 9 Jun 2021 14:43:51 -0700 Linus Torvalds torvalds@linux-foundation.org wrote:
On Wed, Jun 9, 2021 at 1:52 PM Steven Rostedt rostedt@goodmis.org wrote:
That "sizeof(*entry)" is clearly wrong, because it doesn't take the unsized array into account.
Correct. That's because I forgot that the structure has that empty array :-(
Note that 'sparse' does have the option to warn about odd flexible array uses. Including 'sizeof()'.
You can do something like
CF='-Wflexible-array-sizeof' make C=2 kernel/trace/trace.oand you'll see
kernel/trace/trace.c:1022:17: warning: using sizeof on a flexible structure
alloc = sizeof(*entry) + size + 2; /* possible \n added */
Entry is created via a macro. But I guess that too could use a struct_size().
kernel/trace/trace.c:2645:17: warning: using sizeof on a flexible structure
memset(event, 0, sizeof(*event));
Hah, this is the part that is clearing the first part of the ring_buffer_event structure of that temp buffer. Where it's setting "type_len" to zero. It doesn't care about the extended portion here.
kernel/trace/trace.c:2739:41: warning: using sizeof on a flexible structure
This is the code we are talking about now.
kernel/trace/trace.c:3290:16: warning: using sizeof on a flexible structure
This is like the first one. A macro created the structure, but probably could be switched over.
kernel/trace/trace.c:3350:16: warning: using sizeof on a flexible structure
Same as the first occurrence.
kernel/trace/trace.c:6989:16: warning: using sizeof on a flexible structure
Also the same as the first.
kernel/trace/trace.c:7070:16: warning: using sizeof on a flexible structure
And same as the first.
and I suspect every single one of those should be using 'struct_size()' instead for a sizeof() on the base structure plus some manual arithmetic (or, as in the case of this bug, _without_ the extra arithmetic).
Most of the above are for structures that hold dynamic size strings. But I have no problem converting them to struct_size. But since they are created by FTRACE_ENTRY*() macros in kernel/trace/trace_entries.h, we need to be careful in picking the field to use.
And yeah, it isn't just the tracing code that does this. We have it all over, so that sparse check isn't on by default. Sparse is pretty darn noisy even without it, but it can be worth using that CF='-Wflexible-array-sizeof' on individual files that you want to check.
Could be a task for an intern to do?
-- Steve