[GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Steven Rostedt]

Linus,

tracing: One fix in the histogram code and another take at the __string_len() macro

Working on the histogram code, I found that if you dereference a char
pointer in a trace event that happens to point to user space, it can crash
the kernel, as it does no checks of that pointer. I have code coming that
will do this better, so just remove this ability to treat character
pointers in trace events as stings.

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, this is take 2 of the git pull I sent last time, but this also
includes an actual bug fix in the histogram code. I rebased it, where
the histogram fix is first in case you still have issues with the
__string_len() macro change.

I hope my reply satisfied your issues you had with that patch:

  https://lore.kernel.org/lkml/20210714175633.3b53346a@oasis.local.home/

I agreed with you that the __assign_str_len() macro should have a 
do { } while (0) around it, which I updated and tested.

If you still have an issue with this, you can either just pull the
one fix, or I can send you a new tag for just that fix.

Let me know what you would like me to do.

Thanks!

-- Steve

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


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

Tag SHA1: 04a6869693232b86b6640d2db3c25968336a9670
Head SHA1: 85f666175d522f9f1c089f04ed9af5aa4ec84202


Steven Rostedt (VMware) (2):
      tracing: Do not reference char * as a string in histograms
      tracing: Add trace_event helper macros __string_len() and __assign_str_len()

----
 include/trace/trace_events.h     | 22 ++++++++++++++++++++++
 kernel/trace/trace_events_hist.c |  6 +++---
 2 files changed, 25 insertions(+), 3 deletions(-)
---------------------------
diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index acc17194c160..2ebacf03fba4 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,20 @@ 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)					\
+	do {								\
+		strncpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len); \
+		__get_str(dst)[len] = '\0';				\
+	} while(0)
+
 #undef __bitmask
 #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
 
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 0207aeed31e6..16a9dfc9fffc 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1689,7 +1689,9 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
 	if (WARN_ON_ONCE(!field))
 		goto out;
 
-	if (is_string_field(field)) {
+	/* Pointers to strings are just pointers and dangerous to dereference */
+	if (is_string_field(field) &&
+	    (field->filter_type != FILTER_PTR_STRING)) {
 		flags |= HIST_FIELD_FL_STRING;
 
 		hist_field->size = MAX_FILTER_STR_VAL;
@@ -4495,8 +4497,6 @@ static inline void add_to_key(char *compound_key, void *key,
 		field = key_field->field;
 		if (field->filter_type == FILTER_DYN_STRING)
 			size = *(u32 *)(rec + field->offset) >> 16;
-		else if (field->filter_type == FILTER_PTR_STRING)
-			size = strlen(key);
 		else if (field->filter_type == FILTER_STATIC_STRING)
 			size = field->size;
 

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Linus Torvalds]

On Thu, Jul 15, 2021 at 6:57 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> tracing: One fix in the histogram code and another take at the __string_len() macro

What part of "strncpy()" is garbage did I not make clear?

            Linus

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Steven Rostedt]

On Fri, 16 Jul 2021 11:11:38 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Jul 15, 2021 at 6:57 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > tracing: One fix in the histogram code and another take at the __string_len() macro  
> 
> What part of "strncpy()" is garbage did I not make clear?

So how do you want this implemented?

#define __assign_str_len(dst, src, len)					\
	do {								\
		strscpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len); \
		__get_str(dst)[len] = '\0';				\
	} while(0)

I could even do:

#define __assign_str_len(dst, src, len)					\
	do {								\
		if (!src && len > 6)					\
			len = 6;					\
		memcpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len); \
		__get_str(dst)[len] = '\0';				\
	} while(0)

Which would work just as well, although I had to account if len is
greater than "(null)". Which should never happen, but I have it there,
because I copied the code from the __string() version which uses
strlen() and that would break if a null is passed in (which in rare
cases happen). But it would actually be a bug to use __string_len() in
a place that can take a NULL, unless len was zero.

Not sure how the above is any different than using strncpy().

Again, src is a string without a '\0'. What I don't understand is how
strscpy() is any better than strncpy() in this situation?

As I replied to you last time, the dst is created by allocating 'len +
1' on the ring buffer, and len is to be no greater than the number of
characters in src.

The only purpose to use this is to either truncate a string, or to pass
in a string that was read from a memory location that does not have a
terminating '\0' in it, but you know the length of the string. If you
have a normal string, simply use the __string() which uses strlen() to
calculate the required space.

It's basically this:

	dst = malloc(len + 1);
	memcpy(dst, src, len);
	dst[len] = '\0';

"strncpy() is garbage" does not compute to me in the above usage.

-- Steve

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Linus Torvalds]

On Fri, Jul 16, 2021 at 11:37 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> So how do you want this implemented?
>
> #define __assign_str_len(dst, src, len)                                 \
>         do {                                                            \
>                 strscpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len); \
>                 __get_str(dst)[len] = '\0';

What? That "__get_str(dst)[len] = '\0';" is pointless and wrong.

That's the _point_. strscpy() does the whole NUL termination
correctly, in ways that strncpy() never ever did.

But I also want to know what the actual _semantics_ of the source is.
Your "memcpy()" example implies that the source is always a fixed-size
thing. In that case, maybe that's the rigth thing to do, and you
should just create a real function for it.

So two choices:

 (a) either just plain strscpy() works (or, if you want NUL padding,
use strscpy_pad()).

 (b) you have very odd source/destination semantics, and it should be
its own function that explains it.

Note how in neither case is it ok to just make random inline code with
no explanations for the odd crazy code. Make a function that actually
describes what you want, documents it, and be done with it.

strncpy() is garbage. It should never be used in new code.

And random semantics that are undocumented and just implemented as a
illegible mess in a header file is not ok either.

                Linus

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Steven Rostedt]

On Fri, 16 Jul 2021 11:45:57 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Jul 16, 2021 at 11:37 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >
> > So how do you want this implemented?
> >
> > #define __assign_str_len(dst, src, len)                                 \
> >         do {                                                            \
> >                 strscpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len); \
> >                 __get_str(dst)[len] = '\0';
> 
> What? That "__get_str(dst)[len] = '\0';" is pointless and wrong.

I wrote up explanations to all this, and when I finally went to show an
example of where this would be used, I found a major bug, that
questions whether this is needed or not.

Chuck, WTH!

This feature was going to be used by Chuck, because he said he had strings
that had to be saved that did not have terminating nul bytes.

For example, he has:

fs/nfsd/trace.h:

> DECLARE_EVENT_CLASS(nfsd_clid_class,
> 	TP_PROTO(const struct nfsd_net *nn,
> 		 unsigned int namelen,
> 		 const unsigned char *namedata),
> 	TP_ARGS(nn, namelen, namedata),

Above, namedata supposedly has no terminating '\0' byte,
and namelen is the number of characters in namedata.

> 	TP_STRUCT__entry(
> 		__field(unsigned long long, boot_time)
> 		__field(unsigned int, namelen)
> 		__dynamic_array(unsigned char,  name, namelen)

__dynamic_array() allocates __entry->name on the ring buffer of namelen
bytes.

Where my patch would add instead:

		__string(name, namelen)

Which would allocate __entry->name on the ring buffer with "namelen" + 1
bytes.


> 	),
> 	TP_fast_assign(
> 		__entry->boot_time = nn->boot_time;
> 		__entry->namelen = namelen;
> 		memcpy(__get_dynamic_array(name), namedata, namelen);

The above is basically the open coded version of my __assign_str_len(),
where we could use.

		__assign_str_len(name, namedata, namelen);

instead.

> 	),
> 	TP_printk("boot_time=%16llx nfs4_clientid=%.*s",
> 		__entry->boot_time, __entry->namelen, __get_str(name))
> )


With my helpers, Chuck would no longer need this "%.*s", and pass in
__entry->namelen, because, the __assign_str_len() would have added the
'\0' terminating byte,  and "%s" would be sufficient.

But this isn't the example I original used. The example I was going to
use questions Chuck's use case, and was this:

> TRACE_EVENT(nfsd_dirent,
> 	TP_PROTO(struct svc_fh *fhp,
> 		 u64 ino,
> 		 const char *name,
> 		 int namlen),
> 	TP_ARGS(fhp, ino, name, namlen),
> 	TP_STRUCT__entry(
> 		__field(u32, fh_hash)
> 		__field(u64, ino)
> 		__field(int, len)
> 		__dynamic_array(unsigned char, name, namlen)
> 	),
> 	TP_fast_assign(
> 		__entry->fh_hash = fhp ? knfsd_fh_hash(&fhp->fh_handle) : 0;
> 		__entry->ino = ino;
> 		__entry->len = namlen;
> 		memcpy(__get_str(name), name, namlen);

Everything up to here is the same as above, but then there's ...

> 		__assign_str(name, name);

WTH! Chuck, do you know the above expands to:

	strcpy(__get_str(name), (name) ? (const char *)(name) : "(null)");

If "name" does not have a terminating '\0' byte, this would crash hard.

Even if it did have that byte, the __dynamic_array() above only
allocated "namelen" bytes, and that did not include the terminating
byte, which means you are guaranteed to overflow.

It may not have crashed for you if name is nul terminated, because the
ring buffer rounds up to 4 byte alignment,  and you may have had some
extra bytes to use at the end of the event allocation.

But this makes me question if name is really not terminated, and is
this patch actually necessary.

> 	),
> 	TP_printk("fh_hash=0x%08x ino=%llu name=%.*s",
> 		__entry->fh_hash, __entry->ino,
> 		__entry->len, __get_str(name))
> )

I'm dropping this patch for now, and will send another pull request
with just the histogram bug fix.

Thanks,

-- Steve

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Chuck Lever III]

> On Jul 16, 2021, at 5:18 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Fri, 16 Jul 2021 11:45:57 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Fri, Jul 16, 2021 at 11:37 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>>> 
>>> 
>>> So how do you want this implemented?
>>> 
>>> #define __assign_str_len(dst, src, len)                                 \
>>>        do {                                                            \
>>>                strscpy(__get_str(dst), (src) ? (const char *)(src) : "(null)", len); \
>>>                __get_str(dst)[len] = '\0';
>> 
>> What? That "__get_str(dst)[len] = '\0';" is pointless and wrong.
> 
> I wrote up explanations to all this, and when I finally went to show an
> example of where this would be used, I found a major bug, that
> questions whether this is needed or not.
> 
> Chuck, WTH!
> 
> This feature was going to be used by Chuck, because he said he had strings
> that had to be saved that did not have terminating nul bytes.
> 
> For example, he has:
> 
> fs/nfsd/trace.h:
> 
>> DECLARE_EVENT_CLASS(nfsd_clid_class,
>> 	TP_PROTO(const struct nfsd_net *nn,
>> 		 unsigned int namelen,
>> 		 const unsigned char *namedata),
>> 	TP_ARGS(nn, namelen, namedata),
> 
> Above, namedata supposedly has no terminating '\0' byte,
> and namelen is the number of characters in namedata.
> 
>> 	TP_STRUCT__entry(
>> 		__field(unsigned long long, boot_time)
>> 		__field(unsigned int, namelen)
>> 		__dynamic_array(unsigned char,  name, namelen)
> 
> __dynamic_array() allocates __entry->name on the ring buffer of namelen
> bytes.
> 
> Where my patch would add instead:
> 
> 		__string(name, namelen)

You mean

                 __string_len(name, namelen)


> Which would allocate __entry->name on the ring buffer with "namelen" + 1
> bytes.
> 
> 
>> 	),
>> 	TP_fast_assign(
>> 		__entry->boot_time = nn->boot_time;
>> 		__entry->namelen = namelen;
>> 		memcpy(__get_dynamic_array(name), namedata, namelen);
> 
> The above is basically the open coded version of my __assign_str_len(),
> where we could use.
> 
> 		__assign_str_len(name, namedata, namelen);
> 
> instead.
> 
>> 	),
>> 	TP_printk("boot_time=%16llx nfs4_clientid=%.*s",
>> 		__entry->boot_time, __entry->namelen, __get_str(name))
>> )
> 
> 
> With my helpers, Chuck would no longer need this "%.*s", and pass in
> __entry->namelen, because, the __assign_str_len() would have added the
> '\0' terminating byte,  and "%s" would be sufficient.

Exactly, I would still like to do this. I've been waiting
for two months for the __string_len() macros to land.


> But this isn't the example I original used. The example I was going to
> use questions Chuck's use case, and was this:
> 
>> TRACE_EVENT(nfsd_dirent,
>> 	TP_PROTO(struct svc_fh *fhp,
>> 		 u64 ino,
>> 		 const char *name,
>> 		 int namlen),
>> 	TP_ARGS(fhp, ino, name, namlen),
>> 	TP_STRUCT__entry(
>> 		__field(u32, fh_hash)
>> 		__field(u64, ino)
>> 		__field(int, len)
>> 		__dynamic_array(unsigned char, name, namlen)
>> 	),
>> 	TP_fast_assign(
>> 		__entry->fh_hash = fhp ? knfsd_fh_hash(&fhp->fh_handle) : 0;
>> 		__entry->ino = ino;
>> 		__entry->len = namlen;
>> 		memcpy(__get_str(name), name, namlen);
> 
> Everything up to here is the same as above, but then there's ...
> 
>> 		__assign_str(name, name);
> 
> WTH! Chuck, do you know the above expands to:
> 
> 	strcpy(__get_str(name), (name) ? (const char *)(name) : "(null)");
> 
> If "name" does not have a terminating '\0' byte, this would crash hard.

Yes, it does crash hard. That's why I sent this fix:

7b08cf62b123 ("NFSD: Prevent a possible oops in the nfs_dirent() tracepoint")

Which is now in v5.14-rc1 (and should be picked soon up by
automation for backport). I intended to fix nfs_dirent to use
__string_len() and friends, but you decided to delay adding
these new macros, and I had to send the above fix instead.


> Even if it did have that byte, the __dynamic_array() above only
> allocated "namelen" bytes, and that did not include the terminating
> byte, which means you are guaranteed to overflow.
> 
> It may not have crashed for you if name is nul terminated, because the
> ring buffer rounds up to 4 byte alignment,  and you may have had some
> extra bytes to use at the end of the event allocation.
> 
> But this makes me question if name is really not terminated, and is
> this patch actually necessary.

Yes, it is necessary to finish this work.


>> 	),
>> 	TP_printk("fh_hash=0x%08x ino=%llu name=%.*s",
>> 		__entry->fh_hash, __entry->ino,
>> 		__entry->len, __get_str(name))
>> )
> 
> I'm dropping this patch for now,

Please don't drop it. I'm sure these two are not the only uses
for a proper __string_len(). The point of this exercise is to
provide helpers that do all of this manipulation correctly so
that others don't have to take the chance of getting it wrong.


> and will send another pull request with just the histogram bug fix.
> 
> Thanks,
> 
> -- Steve

--
Chuck Lever

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Steven Rostedt]

On Sat, 17 Jul 2021 00:22:52 +0000
Chuck Lever III <chuck.lever@oracle.com> wrote:
> >   
> >> 	TP_STRUCT__entry(
> >> 		__field(unsigned long long, boot_time)
> >> 		__field(unsigned int, namelen)
> >> 		__dynamic_array(unsigned char,  name, namelen)  
> > 
> > __dynamic_array() allocates __entry->name on the ring buffer of namelen
> > bytes.
> > 
> > Where my patch would add instead:
> > 
> > 		__string(name, namelen)  
> 
> You mean
> 
>                  __string_len(name, namelen)

Yes.

> 
> 
> > Which would allocate __entry->name on the ring buffer with "namelen" + 1
> > bytes.
> > 
> >   
> >> 	),
> >> 	TP_fast_assign(
> >> 		__entry->boot_time = nn->boot_time;
> >> 		__entry->namelen = namelen;
> >> 		memcpy(__get_dynamic_array(name), namedata, namelen);  
> > 
> > The above is basically the open coded version of my __assign_str_len(),
> > where we could use.
> > 
> > 		__assign_str_len(name, namedata, namelen);
> > 
> > instead.
> >   
> >> 	),
> >> 	TP_printk("boot_time=%16llx nfs4_clientid=%.*s",
> >> 		__entry->boot_time, __entry->namelen, __get_str(name))
> >> )  
> > 
> > 
> > With my helpers, Chuck would no longer need this "%.*s", and pass in
> > __entry->namelen, because, the __assign_str_len() would have added the
> > '\0' terminating byte,  and "%s" would be sufficient.  
> 
> Exactly, I would still like to do this. I've been waiting
> for two months for the __string_len() macros to land.
> 
> 
> > But this isn't the example I original used. The example I was going to
> > use questions Chuck's use case, and was this:
> >   
> >> TRACE_EVENT(nfsd_dirent,
> >> 	TP_PROTO(struct svc_fh *fhp,
> >> 		 u64 ino,
> >> 		 const char *name,
> >> 		 int namlen),
> >> 	TP_ARGS(fhp, ino, name, namlen),
> >> 	TP_STRUCT__entry(
> >> 		__field(u32, fh_hash)
> >> 		__field(u64, ino)
> >> 		__field(int, len)
> >> 		__dynamic_array(unsigned char, name, namlen)
> >> 	),
> >> 	TP_fast_assign(
> >> 		__entry->fh_hash = fhp ? knfsd_fh_hash(&fhp->fh_handle) : 0;
> >> 		__entry->ino = ino;
> >> 		__entry->len = namlen;
> >> 		memcpy(__get_str(name), name, namlen);  
> > 
> > Everything up to here is the same as above, but then there's ...
> >   
> >> 		__assign_str(name, name);  
> > 
> > WTH! Chuck, do you know the above expands to:
> > 
> > 	strcpy(__get_str(name), (name) ? (const char *)(name) : "(null)");
> > 
> > If "name" does not have a terminating '\0' byte, this would crash hard.  
> 
> Yes, it does crash hard. That's why I sent this fix:

OK, that makes me feel better. I really didn't want this argument with
Linus for nothing ;-)

> 
> 7b08cf62b123 ("NFSD: Prevent a possible oops in the nfs_dirent() tracepoint")
> 
> Which is now in v5.14-rc1 (and should be picked soon up by
> automation for backport). I intended to fix nfs_dirent to use
> __string_len() and friends, but you decided to delay adding
> these new macros, and I had to send the above fix instead.
> 
> 
> > Even if it did have that byte, the __dynamic_array() above only
> > allocated "namelen" bytes, and that did not include the terminating
> > byte, which means you are guaranteed to overflow.
> > 
> > It may not have crashed for you if name is nul terminated, because the
> > ring buffer rounds up to 4 byte alignment,  and you may have had some
> > extra bytes to use at the end of the event allocation.
> > 
> > But this makes me question if name is really not terminated, and is
> > this patch actually necessary.  
> 
> Yes, it is necessary to finish this work.
> 
> 
> >> 	),
> >> 	TP_printk("fh_hash=0x%08x ino=%llu name=%.*s",
> >> 		__entry->fh_hash, __entry->ino,
> >> 		__entry->len, __get_str(name))
> >> )  
> > 
> > I'm dropping this patch for now,  
> 
> Please don't drop it. I'm sure these two are not the only uses
> for a proper __string_len(). The point of this exercise is to
> provide helpers that do all of this manipulation correctly so
> that others don't have to take the chance of getting it wrong.
> 
> 

How about this. I'll just give you the patch and you can apply it to
your tree. I updated it with documentation, and use memcpy instead of
strncpy() as it is replacing memcpy() and strncpy() will cause people
to question the code (as Linus has).

Here's my latest patch. Feel free to apply it to your tree. Hopefully
it wont conflict with other work I'm doing. But if it does, we'll work
it out. I don't have any code that relies on it.

-- Steve

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Subject: [PATCH] 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.

Note, the TRACE_EVENT() macro will allocate the location on the ring
buffer to 'len + 1', that will be used to store the string into. It is a
requirement that the 'len' used for this is a most the length of the
string being recorded.

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>
---
 include/trace/trace_events.h               | 22 ++++++++++++++++++
 samples/trace_events/trace-events-sample.h | 27 ++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
index acc17194c160..08810a463880 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,20 @@ 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)					\
+	do {								\
+		memcpy(__get_str(dst), (src), (len));			\
+		__get_str(dst)[len] = '\0';				\
+	} while(0)
+
 #undef __bitmask
 #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
 
diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h
index 13a35f7cbe66..e61471ab7d14 100644
--- a/samples/trace_events/trace-events-sample.h
+++ b/samples/trace_events/trace-events-sample.h
@@ -141,6 +141,33 @@
  *         In most cases, the __assign_str() macro will take the same
  *         parameters as the __string() macro had to declare the string.
  *
+ *   __string_len: This is a helper to a __dynamic_array, but it understands
+ *	   that the array has characters in it, and with the combined
+ *         use of __assign_str_len(), it will allocate 'len' + 1 bytes
+ *         in the ring buffer and add a '\0' to the string. This is
+ *         useful if the string being saved has no terminating '\0' byte.
+ *         It requires that the length of the string is known as it acts
+ *         like a memcpy().
+ *
+ *         Declared with:
+ *
+ *         __string_len(foo, bar, len)
+ *
+ *         To assign this string, use the helper macro __assign_str_len().
+ *
+ *         __assign_str(foo, bar, len);
+ *
+ *         Then len + 1 is allocated to the ring buffer, and a nul terminating
+ *         byte is added. This is similar to:
+ *
+ *         memcpy(__get_str(foo), bar, len);
+ *         __get_str(foo)[len] = 0;
+ *
+ *        The advantage of using this over __dynamic_array, is that it
+ *        takes care of allocating the extra byte on the ring buffer
+ *        for the '\0' terminating byte, and __get_str(foo) can be used
+ *        in the TP_printk().
+ *
  *   __bitmask: This is another kind of __dynamic_array, but it expects
  *         an array of longs, and the number of bits to parse. It takes
  *         two parameters (name, nr_bits), where name is the name of the
-- 
2.31.1

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Chuck Lever III]

> On Jul 16, 2021, at 8:55 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> How about this. I'll just give you the patch and you can apply it to
> your tree. I updated it with documentation, and use memcpy instead of
> strncpy() as it is replacing memcpy() and strncpy() will cause people
> to question the code (as Linus has).
> 
> Here's my latest patch. Feel free to apply it to your tree.

Thanks. I can carry whichever version of this patch that both you
and Linus are happy with. I will apply my changes to use __string_len()
on top of it, and send the whole mess when v5.15 opens in a couple
months.


> Hopefully
> it wont conflict with other work I'm doing. But if it does, we'll work
> it out. I don't have any code that relies on it.
> 
> -- Steve
> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Subject: [PATCH] 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.
> 
> Note, the TRACE_EVENT() macro will allocate the location on the ring
> buffer to 'len + 1', that will be used to store the string into. It is a
> requirement that the 'len' used for this is a most the length of the
> string being recorded.
> 
> 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>
> ---
> include/trace/trace_events.h               | 22 ++++++++++++++++++
> samples/trace_events/trace-events-sample.h | 27 ++++++++++++++++++++++
> 2 files changed, 49 insertions(+)
> 
> diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h
> index acc17194c160..08810a463880 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,20 @@ 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)					\
> +	do {								\
> +		memcpy(__get_str(dst), (src), (len));			\
> +		__get_str(dst)[len] = '\0';				\
> +	} while(0)
> +
> #undef __bitmask
> #define __bitmask(item, nr_bits) __dynamic_array(unsigned long, item, -1)
> 
> diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h
> index 13a35f7cbe66..e61471ab7d14 100644
> --- a/samples/trace_events/trace-events-sample.h
> +++ b/samples/trace_events/trace-events-sample.h
> @@ -141,6 +141,33 @@
>  *         In most cases, the __assign_str() macro will take the same
>  *         parameters as the __string() macro had to declare the string.
>  *
> + *   __string_len: This is a helper to a __dynamic_array, but it understands
> + *	   that the array has characters in it, and with the combined
> + *         use of __assign_str_len(), it will allocate 'len' + 1 bytes
> + *         in the ring buffer and add a '\0' to the string. This is
> + *         useful if the string being saved has no terminating '\0' byte.
> + *         It requires that the length of the string is known as it acts
> + *         like a memcpy().
> + *
> + *         Declared with:
> + *
> + *         __string_len(foo, bar, len)
> + *
> + *         To assign this string, use the helper macro __assign_str_len().
> + *
> + *         __assign_str(foo, bar, len);
> + *
> + *         Then len + 1 is allocated to the ring buffer, and a nul terminating
> + *         byte is added. This is similar to:
> + *
> + *         memcpy(__get_str(foo), bar, len);
> + *         __get_str(foo)[len] = 0;
> + *
> + *        The advantage of using this over __dynamic_array, is that it
> + *        takes care of allocating the extra byte on the ring buffer
> + *        for the '\0' terminating byte, and __get_str(foo) can be used
> + *        in the TP_printk().
> + *
>  *   __bitmask: This is another kind of __dynamic_array, but it expects
>  *         an array of longs, and the number of bits to parse. It takes
>  *         two parameters (name, nr_bits), where name is the name of the
> -- 
> 2.31.1
> 

--
Chuck Lever

Re: [GIT PULL] tracing: histogram fix and take 2 on the __string_len() marcros

[Steven Rostedt]

On Sat, 17 Jul 2021 16:51:58 +0000
Chuck Lever III <chuck.lever@oracle.com> wrote:

> Thanks. I can carry whichever version of this patch that both you
> and Linus are happy with. I will apply my changes to use __string_len()
> on top of it, and send the whole mess when v5.15 opens in a couple
> months.

The version in this email is the one that I believe is something that
myself and Linus can live with. It uses memcpy() as it replaces a
memcpy() and I added documentation about it in the
samples/trace_events/ directory.

-- Steve


> 
> 
> > Hopefully
> > it wont conflict with other work I'm doing. But if it does, we'll work
> > it out. I don't have any code that relies on it.
> > 
> > -- Steve
> > 
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > Subject: [PATCH] tracing: Add trace_event helper macros __string_len() and
> > __assign_str_len()
> >