[GIT PULL] tracing: Add __string_len() and __assign_str_len() helpers

[GIT PULL] tracing: Add __string_len() and __assign_str_len() helpers

[Steven Rostedt]

Linus,

Add macros for the TRACE_EVENT() macro that can be used to assign strings
that either need to be truncated, or have no nul terminator, and depends
on a length attribute to assign.

Note: A while ago Chuck Lever asked for these changes, and I sent him this
patch to see if it would work for him, and he said it would. But because
I never made a commit for it, I forgot about it. Although this is not
an actual fix, it also has no functional changes. It introduces two
macro helpers to the TRACE_EVENT() macros, and that is all it does.
These can then be used by Chuck for work that will go into the next
merge window.

It would be easier if it were upstream instead of him carrying the change,
and likely conflict with work I am working on as this touches the core
file that creates the TRACE_EVENT macro. The other approach is that I make
a specific branch that he can then base off of in his tree, and we both have
that as our base branch. But still, that complicates things. Third approach is
that he takes this patch into his tree, and I take the same patch into mine
and just work on top of it even though they have separate sha1s.

If you do not want to pull this because this is not technically a fix, we
will then just go with one of the above approaches. I'll let it be your call.

Please pull the latest trace-v5.14-3 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v5.14-3

Tag SHA1: 7217c1cf4cd4c5f22e7b3bd641a594bbe7b7c8e1
Head SHA1: ac58f4f28369ce3287982da131ad3c6fb283d4e6


Steven Rostedt (VMware) (1):
      tracing: Add trace_event helper macros __string_len() and __assign_str_len()

----
 include/trace/trace_events.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
---------------------------
commit ac58f4f28369ce3287982da131ad3c6fb283d4e6
Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
Date:   Thu May 13 10:50:18 2021 -0400

    tracing: Add trace_event helper macros __string_len() and __assign_str_len()
    
    There's a few cases that a string that is to be recorded in a trace event,
    does not have a terminating 'nul' character, and instead, the tracepoint
    passes in the length of the string to record.
    
    Add two helper macros to the trace event code that lets this work easier,
    than tricks with "%.*s" logic.
    
      __string_len() which is similar to __string() for declaration, but takes a
                     length argument.
    
      __assign_str_len() which is similar to __assign_str() for assiging the
                     string, but it too takes a length argument.
    
    This string can still use __get_str() just like strings created with
    __string() can use to retrieve the string.
    
    Link: https://lore.kernel.org/linux-nfs/20210513105018.7539996a@gandalf.local.home/
    
    Tested-by: Chuck Lever <chuck.lever@oracle.com>
    Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index acc17194c160..a0fa8a3a691c 100644
--- a/include/trace/trace_events.h
+++ b/include/trace/trace_events.h
@@ -102,6 +102,9 @@ TRACE_MAKE_SYSTEM_STR();
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
 
+#undef __string_len
+#define __string_len(item, src, len) __dynamic_array(char, item, -1)
+
 #undef __bitmask
 #define __bitmask(item, nr_bits) __dynamic_array(char, item, -1)
 
@@ -197,6 +200,9 @@ TRACE_MAKE_SYSTEM_STR();
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
 
+#undef __string_len
+#define __string_len(item, src, len) __dynamic_array(char, item, -1)
+
 #undef __bitmask
 #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
 
@@ -459,6 +465,9 @@ static struct trace_event_functions trace_event_type_funcs_##call = {	\
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
 
+#undef __string_len
+#define __string_len(item, src, len) __dynamic_array(char, item, -1)
+
 #undef __bitmask
 #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
 
@@ -507,6 +516,9 @@ static struct trace_event_fields trace_event_fields_##call[] = {	\
 #define __string(item, src) __dynamic_array(char, item,			\
 		    strlen((src) ? (const char *)(src) : "(null)") + 1)
 
+#undef __string_len
+#define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)
+
 /*
  * __bitmask_size_in_bytes_raw is the number of bytes needed to hold
  * num_possible_cpus().
@@ -670,10 +682,18 @@ static inline notrace int trace_event_get_offsets_##call(		\
 #undef __string
 #define __string(item, src) __dynamic_array(char, item, -1)
 
+#undef __string_len
+#define __string_len(item, src, len) __dynamic_array(char, item, -1)
+
 #undef __assign_str
 #define __assign_str(dst, src)						\
 	strcpy(__get_str(dst), (src) ? (const char *)(src) : "(null)");
 
+#undef __assign_str_len
+#define __assign_str_len(dst, src, len)						\
+	strncpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len);	\
+	__get_str(dst)[len] = '\0';
+
 #undef __bitmask
 #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
 

Re: [GIT PULL] tracing: Add __string_len() and __assign_str_len() helpers

[Linus Torvalds]

On Tue, Jul 13, 2021 at 2:11 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Add macros for the TRACE_EVENT() macro that can be used to assign strings
> that either need to be truncated, or have no nul terminator, and depends
> on a length attribute to assign.

I pulled this, but then I looked at the actual patch, and decided it's
not acceptable.

> +#define __assign_str_len(dst, src, len)                                                \
> +       strncpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len);   \
> +       __get_str(dst)[len] = '\0';

I can see so many problems in the above that it's not even funny.

Maybe all users would end up avoiding the pitfalls, but the above
really is disgusting.

And yes, there's a pre-existing multi-statement macro without any
grouping, but that's not an excuse for doing more of them, and doing
them badly.

And by "badly" I mean - among other things - the questionable NUL
termination that *overflows* the size that was specified, but also
using strncpy() at all.

Hint: use strscpy instead of re-implementing it badly. If you really
want the crazy NUL padding that strncpy does - which I doubt you do -
use strscpy_pad(), making it explicit.

             Linus

Re: [GIT PULL] tracing: Add __string_len() and __assign_str_len() helpers

[Steven Rostedt]

On Wed, 14 Jul 2021 12:20:47 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Jul 13, 2021 at 2:11 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Add macros for the TRACE_EVENT() macro that can be used to assign strings
> > that either need to be truncated, or have no nul terminator, and depends
> > on a length attribute to assign.  
> 
> I pulled this, but then I looked at the actual patch, and decided it's
> not acceptable.
> 
> > +#define __assign_str_len(dst, src, len)                                                \
> > +       strncpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len);   \
> > +       __get_str(dst)[len] = '\0';  
> 
> I can see so many problems in the above that it's not even funny.

It's part of the ugliness of the TRACE_EVENT() macro helpers, as these
macros should never be used outside of the TP_fast_assign() logic.
They're internal helper macros, that have a lot of assumptions. It's
probably time to add a bunch of comments to them.

> 
> Maybe all users would end up avoiding the pitfalls, but the above
> really is disgusting.
> 
> And yes, there's a pre-existing multi-statement macro without any
> grouping, but that's not an excuse for doing more of them, and doing
> them badly.

Adding the "do { } while(0)" is probably a good idea here. I haven't
thought it necessary because of the limited use cases. But I have seen
people get crazy inside the TP_fast_assign(), so it probably should be
included. But other than that, I don't see any issues.

> 
> And by "badly" I mean - among other things - the questionable NUL
> termination that *overflows* the size that was specified, but also
> using strncpy() at all.
> 
> Hint: use strscpy instead of re-implementing it badly. If you really
> want the crazy NUL padding that strncpy does - which I doubt you do -
> use strscpy_pad(), making it explicit.

Note, the source is guaranteed to be a smaller buffer than the
destination. The destination is allocated to len + 1, we only want to
copy the full source string, which does not have a terminating '\0',
and add it to a string that we will add a terminating '\0' too.

In other words, strscpy is not any better, because we have to add one
more anyway.

If you look at how that is allocated, we have:

+#undef __string_len
+#define __string_len(item, src, len) __dynamic_array(char, item, (len) + 1)


That "(len)+1" adds the extra byte for the '0'. Hence, it is similar to:

	dst = kmalloc(len + 1, GFP_KERNEL);
	strncpy(dst, src, len);
	dst[len] = '\0';

Where the requirement is that you want to save len bytes of source into
dst, where len must be at least the size of src.

The use case for such a thing is:

TRACE_EVENT(foo,
	TP_PROTO(const char *foobar, int len),
	TP_STRUCT__entry(
		__string_len(	str,	foobar,		len)
	),
	TP_fast_assign(
		__assign_str_len(str, foobar, len);
	),
	TP_printk("str=%s", __get_str(str))
);


This creates the event "foo" where in the kernel proper, you have:

	trace_foo(myfoo, myfoo_len);

Where myfoo_len, is known to be all the characters to copy from myfoo
(where myfoo has no terminator)

The TRACE_EVENT macro turns into:

 I manually substituted the __string_len() and __assign_str_len() macros
 to show what the functions end up as.

(created by the macro magic in include/trace/trace_events.h)


// trace_event_get_offsets_foo is used to calculate the dynamic fields
// of the trace event, and figure out where they will be located.

static inline notrace int trace_event_get_offsets_foo(
	struct trace_event_data_offsets_foo *__data_offsets, const char *foobar, int len)
{
	int __data_size = 0;
	int __maybe_unused __item_length;
	struct trace_event_raw_foo __maybe_unused *entry;

// The "(len)+1" will be used to calculate the __item_length below

	__item_length = (len + 1) * sizeof(char);

// the dynamic fields (strings and such) are located after the static
// fields in the trace event, and the offset is recorded in the bottom
// 16 bits of a 32 bit word, and the size in the top 16 bits.

	__data_offsets->item = __data_size +
			       offsetof(typeof(*entry), __data);
	__data_offsets->item |= __item_length << 16;
	__data_size += __item_length;


// Each "__string_len()" or "__string()" in the TP_STRUCT__entry is inserted
// into this macro created function. Thus, the above code is duplicated for
// each field that uses __string(), __string_len() or __dynamic_array().


	return __data_size;
}

#define __get_str(str)	\
		((void *)__entry + (__entry->__data_loc_str & 0xffff))

// The trace_event_raw_event_foo() is called by the trace_foo()
// when the trace foo event is enabled (via static branch).

static notrace void
trace_event_raw_event_foo(void *__data, const char **foobar, int len)
{
	struct trace_event_file *trace_file = __data;
	struct trace_event_data_offsets_foo __maybe_unused __data_offsets;
	struct trace_event_buffer fbuffer;
	struct trace_event_raw_foo *entry;
	int __data_size;

// First the size is calculated from the above code to find out how much
// needs to be allocated on the ring buffer.

	__data_size = trace_event_get_offsets_foo(&__data_offsets, foobar, len);

	entry = trace_event_buffer_reserve(&fbuffer, trace_file,
				 sizeof(*entry) + __data_size);

	if (!entry)
		return;

// The below is the __assign_str_len() that is in the TP_fast_assign() which
// is expanded here.

	strncpy(__get_str(str), (foorbar) ? (const char *)(foobar) : "(null)", len);
	__get_str(str)[len] = '\0';

// The above will not overflow, as the caller is expected to pass in a len
// that has fewer than or equal to characters of the string (or that is a bug).
// The destination was allocated as len + 1, so it will not overflow

	trace_event_buffer_commit(&fbuffer);
}

There is no overflow, because the __string_len() automatically
allocated len+1, and len is used here.

I agree that for __assign_str_len() that it should have a do { }
while(0) because someone could have:

	TP_fast_assign(
		if (len > 10)
			__assign_str_len(str, foobar, 10);
		else
			__assign_str_len(str, foobar, len);

and this would break with the current logic.

I admit TRACE_EVENT() logic is rather ugly, but it was done this way to
make it really easy to create events, otherwise, people would need to
do all this by hand, and that would be difficult and error prone. The
fact that we have over 1000 events proves that it was easy ;-)

But, besides wrapping the __assign_str_len() in a do { } while (0), I
don't see an overflow or other bug in this code.

-- Steve

Re: [GIT PULL] tracing: Add __string_len() and __assign_str_len() helpers

[Steven Rostedt]

On Wed, 14 Jul 2021 17:56:33 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> static inline notrace int trace_event_get_offsets_foo(
> 	struct trace_event_data_offsets_foo *__data_offsets, const char *foobar, int len)
> {
> 	int __data_size = 0;
> 	int __maybe_unused __item_length;
> 	struct trace_event_raw_foo __maybe_unused *entry;
> 
> // The "(len)+1" will be used to calculate the __item_length below
> 
> 	__item_length = (len + 1) * sizeof(char);
> 
> // the dynamic fields (strings and such) are located after the static
> // fields in the trace event, and the offset is recorded in the bottom
> // 16 bits of a 32 bit word, and the size in the top 16 bits.
> 
> 	__data_offsets->item = __data_size +
> 			       offsetof(typeof(*entry), __data);
> 	__data_offsets->item |= __item_length << 16;

I missed a manual substitution of the macro. This should have been:

 	__data_offsets->str = __data_size +
 			       offsetof(typeof(*entry), __data);
 	__data_offsets->str |= __item_length << 16;

As the macro creates the trace_event_data_offsets_foo structure (which
__data_offsets is declared as), which contains a field for every
__string/__string_len/__dynamic_array() in TP_STRUCT__entry.

The -">str" comes from the "str" in "__string_len(str, foobar, len)".

-- Steve


> 	__data_size += __item_length;
> 
> 
> // Each "__string_len()" or "__string()" in the TP_STRUCT__entry is inserted
> // into this macro created function. Thus, the above code is duplicated for
> // each field that uses __string(), __string_len() or __dynamic_array().
> 
> 
> 	return __data_size;
> }

Re: [GIT PULL] tracing: Add __string_len() and __assign_str_len() helpers

[Steven Rostedt]

On Wed, 14 Jul 2021 17:56:33 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> That "(len)+1" adds the extra byte for the '0'. Hence, it is similar to:
> 
> 	dst = kmalloc(len + 1, GFP_KERNEL);
> 	strncpy(dst, src, len);
> 	dst[len] = '\0';
> 
> Where the requirement is that you want to save len bytes of source into
> dst, where len must be at least the size of src.

Sorry, that should have read "len must be at most the size of src".

That is, the caller knows how big src is, and is telling the trace
event that all characters up to "len" exists.

-- Steve