| Message ID | 162083370248.3108.7424008399973918267.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD callback and lease management observability | expand |
On Wed, 12 May 2021 11:35:02 -0400 Chuck Lever <chuck.lever@oracle.com> wrote: > Since commit 9a6944fee68e ("tracing: Add a verifier to check string > pointers for trace events"), which was merged in v5.13-rc1, > TP_printk() no longer tacitly supports the "%.*s" format specifier. Hmm, this looks like a bug. I should allow the %.*s notation. I probably should fix that. -- Steve
Hi Steve- > On May 12, 2021, at 12:26 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 12 May 2021 11:35:02 -0400 > Chuck Lever <chuck.lever@oracle.com> wrote: > >> Since commit 9a6944fee68e ("tracing: Add a verifier to check string >> pointers for trace events"), which was merged in v5.13-rc1, >> TP_printk() no longer tacitly supports the "%.*s" format specifier. > > Hmm, this looks like a bug. I should allow the %.*s notation. > > I probably should fix that. The underlying need is to support non-NUL-terminated C strings. I assumed that since the commentary around 9a6944fee68e claims the proper way to trace C strings is to use __string and friends, and those do not support non-NUL-terminated strings, that such strings are really not first-class citizens. Thus I concluded that my use of '%.*s' was incorrect. Having some __string-style helpers that can deal with such strings would be valuable. -- Chuck Lever
On Wed, 12 May 2021 12:26:23 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 12 May 2021 11:35:02 -0400 > Chuck Lever <chuck.lever@oracle.com> wrote: > > > Since commit 9a6944fee68e ("tracing: Add a verifier to check string > > pointers for trace events"), which was merged in v5.13-rc1, > > TP_printk() no longer tacitly supports the "%.*s" format specifier. > > Hmm, this looks like a bug. I should allow the %.*s notation. > > I probably should fix that. Only compiled tested, but the %.*s notation should work with this. -- Steve diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index e28d08905124..0181122f1e80 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -3700,6 +3700,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, goto print; while (*p) { + bool star = false; + int len = 0; + j = 0; /* We only care about %s and variants */ @@ -3721,13 +3724,17 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, /* Need to test cases like %08.*s */ for (j = 1; p[i+j]; j++) { if (isdigit(p[i+j]) || - p[i+j] == '*' || p[i+j] == '.') continue; + if (p[i+j] == '*') { + star = true; + continue; + } break; } if (p[i+j] == 's') break; + star = false; } j = 0; } @@ -3740,6 +3747,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, iter->fmt[i] = '\0'; trace_seq_vprintf(&iter->seq, iter->fmt, ap); + if (star) + len = va_arg(ap, int); + /* The ap now points to the string data of the %s */ str = va_arg(ap, const char *); @@ -3758,8 +3768,18 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, int ret; /* Try to safely read the string */ - ret = strncpy_from_kernel_nofault(iter->fmt, str, - iter->fmt_size); + if (star) { + if (len + 1 > iter->fmt_size) + len = iter->fmt_size - 1; + if (len < 0) + len = 0; + ret = copy_from_kernel_nofault(iter->fmt, str, len); + iter->fmt[len] = 0; + star = false; + } else { + ret = strncpy_from_kernel_nofault(iter->fmt, str, + iter->fmt_size); + } if (ret < 0) trace_seq_printf(&iter->seq, "(0x%px)", str); else @@ -3771,7 +3791,10 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, strncpy(iter->fmt, p + i, j + 1); iter->fmt[j+1] = '\0'; } - trace_seq_printf(&iter->seq, iter->fmt, str); + if (star) + trace_seq_printf(&iter->seq, iter->fmt, len, str); + else + trace_seq_printf(&iter->seq, iter->fmt, str); p += i + j + 1; }
On Wed, 12 May 2021 16:52:05 +0000 Chuck Lever III <chuck.lever@oracle.com> wrote: > The underlying need is to support non-NUL-terminated C strings. > > I assumed that since the commentary around 9a6944fee68e claims > the proper way to trace C strings is to use __string and friends, > and those do not support non-NUL-terminated strings, that such > strings are really not first-class citizens. Thus I concluded > that my use of '%.*s' was incorrect. > > Having some __string-style helpers that can deal with such > strings would be valuable. I guess the best I can do is a strncpy version, that will add the '\0' in the ring buffer. That way we don't need to save the length as well (length would need to be at least 4 bytes, where as '\0' is one). Something like this? I added "__string_len()" and "__assign_str_len()". You use them just like __string() and __assign_str() but add a max length that you want to use (although, it will always allocate "len" regardless if the string is smaller). Then use __get_str() just like you use __string(). Would something like that work? -- Steve diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h index 8268bf747d6f..7ab23535a0c8 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) @@ -444,6 +450,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) @@ -492,6 +501,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(). @@ -655,10 +667,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)
> On May 13, 2021, at 10:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 12 May 2021 16:52:05 +0000 > Chuck Lever III <chuck.lever@oracle.com> wrote: > >> The underlying need is to support non-NUL-terminated C strings. >> >> I assumed that since the commentary around 9a6944fee68e claims >> the proper way to trace C strings is to use __string and friends, >> and those do not support non-NUL-terminated strings, that such >> strings are really not first-class citizens. Thus I concluded >> that my use of '%.*s' was incorrect. >> >> Having some __string-style helpers that can deal with such >> strings would be valuable. > > I guess the best I can do is a strncpy version, that will add the '\0' in > the ring buffer. That way we don't need to save the length as well (length > would need to be at least 4 bytes, where as '\0' is one). > > Something like this? That looks about right! With the caveat that a non-NUL-terminated string might contain a NUL. When displayed with '%s', such a string would be truncated. > I added "__string_len()" and "__assign_str_len()". You use them just like > __string() and __assign_str() but add a max length that you want to use > (although, it will always allocate "len" regardless if the string is > smaller). Then use __get_str() just like you use __string(). > > Would something like that work? > > -- Steve > > diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h > index 8268bf747d6f..7ab23535a0c8 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) > > @@ -444,6 +450,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) > > @@ -492,6 +501,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(). > @@ -655,10 +667,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) > > > -- Chuck Lever
On Thu, 13 May 2021 14:53:35 +0000 Chuck Lever III <chuck.lever@oracle.com> wrote: > > Something like this? > > That looks about right! > > With the caveat that a non-NUL-terminated string might contain a NUL. > When displayed with '%s', such a string would be truncated. Right, but that would be the case even with %.*s. So it works the same. -- Steve
> On May 13, 2021, at 10:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 12 May 2021 16:52:05 +0000 > Chuck Lever III <chuck.lever@oracle.com> wrote: > >> The underlying need is to support non-NUL-terminated C strings. >> >> I assumed that since the commentary around 9a6944fee68e claims >> the proper way to trace C strings is to use __string and friends, >> and those do not support non-NUL-terminated strings, that such >> strings are really not first-class citizens. Thus I concluded >> that my use of '%.*s' was incorrect. >> >> Having some __string-style helpers that can deal with such >> strings would be valuable. > > I guess the best I can do is a strncpy version, that will add the '\0' in > the ring buffer. That way we don't need to save the length as well (length > would need to be at least 4 bytes, where as '\0' is one). > > Something like this? > > I added "__string_len()" and "__assign_str_len()". You use them just like > __string() and __assign_str() but add a max length that you want to use > (although, it will always allocate "len" regardless if the string is > smaller). Then use __get_str() just like you use __string(). > > Would something like that work? I will test later today and let you know in this thread. > -- Steve > > diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h > index 8268bf747d6f..7ab23535a0c8 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) > > @@ -444,6 +450,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) > > @@ -492,6 +501,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(). > @@ -655,10 +667,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) > > > -- Chuck Lever
> On May 13, 2021, at 11:10 AM, Chuck Lever III <chuck.lever@oracle.com> wrote: > >> On May 13, 2021, at 10:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote: >> >> On Wed, 12 May 2021 16:52:05 +0000 >> Chuck Lever III <chuck.lever@oracle.com> wrote: >> >>> The underlying need is to support non-NUL-terminated C strings. >>> >>> I assumed that since the commentary around 9a6944fee68e claims >>> the proper way to trace C strings is to use __string and friends, >>> and those do not support non-NUL-terminated strings, that such >>> strings are really not first-class citizens. Thus I concluded >>> that my use of '%.*s' was incorrect. >>> >>> Having some __string-style helpers that can deal with such >>> strings would be valuable. >> >> I guess the best I can do is a strncpy version, that will add the '\0' in >> the ring buffer. That way we don't need to save the length as well (length >> would need to be at least 4 bytes, where as '\0' is one). >> >> Something like this? >> >> I added "__string_len()" and "__assign_str_len()". You use them just like >> __string() and __assign_str() but add a max length that you want to use >> (although, it will always allocate "len" regardless if the string is >> smaller). Then use __get_str() just like you use __string(). >> >> Would something like that work? > > I will test later today and let you know in this thread. All good. Tested-by: Chuck Lever <chuck.lever@oracle.com> -- Chuck Lever
On Thu, 13 May 2021 18:43:29 +0000 Chuck Lever III <chuck.lever@oracle.com> wrote: > >> Would something like that work? > > > > I will test later today and let you know in this thread. > > All good. > > Tested-by: Chuck Lever <chuck.lever@oracle.com> Thanks, since this is an enhancement and not a fix, it will need to wait till the next merge window to go in. I can write up a formal patch and let you add it to this patch set as the first patch, or I can add it to my tree if you don't expect this to get into the next merge window. -- Steve
> On May 13, 2021, at 3:00 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 13 May 2021 18:43:29 +0000 > Chuck Lever III <chuck.lever@oracle.com> wrote: > >>>> Would something like that work? >>> >>> I will test later today and let you know in this thread. >> >> All good. >> >> Tested-by: Chuck Lever <chuck.lever@oracle.com> > > Thanks, since this is an enhancement and not a fix, it will need to wait > till the next merge window to go in. > > I can write up a formal patch and let you add it to this patch set as the > first patch, or I can add it to my tree if you don't expect this to get > into the next merge window. The tracepoints that currently use '%.*s' no longer work when using "trace-cmd start/stop/show". They were working before 9a6944fee68e, so I consider this a regression. I plan to submit patches to address this for 5.13-rc. I guess they will have to go without the use of the new _len macros for now, and you can push the macros in v5.14. -- Chuck Lever
On Thu, 13 May 2021 19:08:13 +0000 Chuck Lever III <chuck.lever@oracle.com> wrote: > The tracepoints that currently use '%.*s' no longer work when > using "trace-cmd start/stop/show". They were working before > 9a6944fee68e, so I consider this a regression. I plan to > submit patches to address this for 5.13-rc. I guess they will > have to go without the use of the new _len macros for now, > and you can push the macros in v5.14. That's a separate bug. I'm currently running this patch through my tests, and will push to Linus when it completes. Feel free to test this one too. -- Steve From eb01f5353bdaa59600b29d864819056a0e3de24d Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Date: Thu, 13 May 2021 12:23:24 -0400 Subject: [PATCH] tracing: Handle %.*s in trace_check_vprintf() If a trace event uses the %*.s notation, the trace_check_vprintf() will fail and will warn about a bad processing of strings, because it does not take into account the length field when processing the star (*) part. Have it handle this case as well. Link: https://lore.kernel.org/linux-nfs/238C0E2D-C2A4-4578-ADD2-C565B3B99842@oracle.com/ Reported-by: Chuck Lever III <chuck.lever@oracle.com> Fixes: 9a6944fee68e2 ("tracing: Add a verifier to check string pointers for trace events") Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/trace.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 560e4c8d3825..a21ef9cd2aae 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -3704,6 +3704,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, goto print; while (*p) { + bool star = false; + int len = 0; + j = 0; /* We only care about %s and variants */ @@ -3725,13 +3728,17 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, /* Need to test cases like %08.*s */ for (j = 1; p[i+j]; j++) { if (isdigit(p[i+j]) || - p[i+j] == '*' || p[i+j] == '.') continue; + if (p[i+j] == '*') { + star = true; + continue; + } break; } if (p[i+j] == 's') break; + star = false; } j = 0; } @@ -3744,6 +3751,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, iter->fmt[i] = '\0'; trace_seq_vprintf(&iter->seq, iter->fmt, ap); + if (star) + len = va_arg(ap, int); + /* The ap now points to the string data of the %s */ str = va_arg(ap, const char *); @@ -3762,8 +3772,18 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, int ret; /* Try to safely read the string */ - ret = strncpy_from_kernel_nofault(iter->fmt, str, - iter->fmt_size); + if (star) { + if (len + 1 > iter->fmt_size) + len = iter->fmt_size - 1; + if (len < 0) + len = 0; + ret = copy_from_kernel_nofault(iter->fmt, str, len); + iter->fmt[len] = 0; + star = false; + } else { + ret = strncpy_from_kernel_nofault(iter->fmt, str, + iter->fmt_size); + } if (ret < 0) trace_seq_printf(&iter->seq, "(0x%px)", str); else @@ -3775,7 +3795,10 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, strncpy(iter->fmt, p + i, j + 1); iter->fmt[j+1] = '\0'; } - trace_seq_printf(&iter->seq, iter->fmt, str); + if (star) + trace_seq_printf(&iter->seq, iter->fmt, len, str); + else + trace_seq_printf(&iter->seq, iter->fmt, str); p += i + j + 1; }
> On May 13, 2021, at 3:17 PM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 13 May 2021 19:08:13 +0000 > Chuck Lever III <chuck.lever@oracle.com> wrote: > >> The tracepoints that currently use '%.*s' no longer work when >> using "trace-cmd start/stop/show". They were working before >> 9a6944fee68e, so I consider this a regression. I plan to >> submit patches to address this for 5.13-rc. I guess they will >> have to go without the use of the new _len macros for now, >> and you can push the macros in v5.14. > > That's a separate bug. I'm currently running this patch through my tests, > and will push to Linus when it completes. Feel free to test this one too. Confirmed that applying the below patch addresses the regression. Tested-by: Chuck Lever <chuck.lever@oracle.com> Thanks for your quick response! > -- Steve > > From eb01f5353bdaa59600b29d864819056a0e3de24d Mon Sep 17 00:00:00 2001 > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> > Date: Thu, 13 May 2021 12:23:24 -0400 > Subject: [PATCH] tracing: Handle %.*s in trace_check_vprintf() > > If a trace event uses the %*.s notation, the trace_check_vprintf() will > fail and will warn about a bad processing of strings, because it does not > take into account the length field when processing the star (*) part. > Have it handle this case as well. > > Link: https://lore.kernel.org/linux-nfs/238C0E2D-C2A4-4578-ADD2-C565B3B99842@oracle.com/ > > Reported-by: Chuck Lever III <chuck.lever@oracle.com> > Fixes: 9a6944fee68e2 ("tracing: Add a verifier to check string pointers for trace events") > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> > --- > kernel/trace/trace.c | 31 +++++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 560e4c8d3825..a21ef9cd2aae 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -3704,6 +3704,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, > goto print; > > while (*p) { > + bool star = false; > + int len = 0; > + > j = 0; > > /* We only care about %s and variants */ > @@ -3725,13 +3728,17 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, > /* Need to test cases like %08.*s */ > for (j = 1; p[i+j]; j++) { > if (isdigit(p[i+j]) || > - p[i+j] == '*' || > p[i+j] == '.') > continue; > + if (p[i+j] == '*') { > + star = true; > + continue; > + } > break; > } > if (p[i+j] == 's') > break; > + star = false; > } > j = 0; > } > @@ -3744,6 +3751,9 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, > iter->fmt[i] = '\0'; > trace_seq_vprintf(&iter->seq, iter->fmt, ap); > > + if (star) > + len = va_arg(ap, int); > + > /* The ap now points to the string data of the %s */ > str = va_arg(ap, const char *); > > @@ -3762,8 +3772,18 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, > int ret; > > /* Try to safely read the string */ > - ret = strncpy_from_kernel_nofault(iter->fmt, str, > - iter->fmt_size); > + if (star) { > + if (len + 1 > iter->fmt_size) > + len = iter->fmt_size - 1; > + if (len < 0) > + len = 0; > + ret = copy_from_kernel_nofault(iter->fmt, str, len); > + iter->fmt[len] = 0; > + star = false; > + } else { > + ret = strncpy_from_kernel_nofault(iter->fmt, str, > + iter->fmt_size); > + } > if (ret < 0) > trace_seq_printf(&iter->seq, "(0x%px)", str); > else > @@ -3775,7 +3795,10 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, > strncpy(iter->fmt, p + i, j + 1); > iter->fmt[j+1] = '\0'; > } > - trace_seq_printf(&iter->seq, iter->fmt, str); > + if (star) > + trace_seq_printf(&iter->seq, iter->fmt, len, str); > + else > + trace_seq_printf(&iter->seq, iter->fmt, str); > > p += i + j + 1; > } > -- > 2.29.2 > -- Chuck Lever
Hello Steven- > On May 13, 2021, at 10:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 12 May 2021 16:52:05 +0000 > Chuck Lever III <chuck.lever@oracle.com> wrote: > >> The underlying need is to support non-NUL-terminated C strings. >> >> I assumed that since the commentary around 9a6944fee68e claims >> the proper way to trace C strings is to use __string and friends, >> and those do not support non-NUL-terminated strings, that such >> strings are really not first-class citizens. Thus I concluded >> that my use of '%.*s' was incorrect. >> >> Having some __string-style helpers that can deal with such >> strings would be valuable. > > I guess the best I can do is a strncpy version, that will add the '\0' in > the ring buffer. That way we don't need to save the length as well (length > would need to be at least 4 bytes, where as '\0' is one). > > Something like this? I don't see this change in v5.14-rc1. > I added "__string_len()" and "__assign_str_len()". You use them just like > __string() and __assign_str() but add a max length that you want to use > (although, it will always allocate "len" regardless if the string is > smaller). Then use __get_str() just like you use __string(). > > Would something like that work? > > -- Steve > > diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h > index 8268bf747d6f..7ab23535a0c8 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) > > @@ -444,6 +450,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) > > @@ -492,6 +501,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(). > @@ -655,10 +667,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) > > > -- Chuck Lever
On Mon, 12 Jul 2021 15:18:05 +0000 Chuck Lever III <chuck.lever@oracle.com> wrote: > Hello Steven- > > > On May 13, 2021, at 10:50 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > > > On Wed, 12 May 2021 16:52:05 +0000 > > Chuck Lever III <chuck.lever@oracle.com> wrote: > > > >> The underlying need is to support non-NUL-terminated C strings. > >> > >> I assumed that since the commentary around 9a6944fee68e claims > >> the proper way to trace C strings is to use __string and friends, > >> and those do not support non-NUL-terminated strings, that such > >> strings are really not first-class citizens. Thus I concluded > >> that my use of '%.*s' was incorrect. > >> > >> Having some __string-style helpers that can deal with such > >> strings would be valuable. > > > > I guess the best I can do is a strncpy version, that will add the '\0' in > > the ring buffer. That way we don't need to save the length as well (length > > would need to be at least 4 bytes, where as '\0' is one). > > > > Something like this? > > I don't see this change in v5.14-rc1. Grumble. I thought this thread was over with the bug fix that was sent, but completely forgot about this change, as I never made it into a real commit. I can add this now, kick off some tests, and grovel to Linus saying that this is unused code, but other code will depend on this, and I want to get it upstream so that it can. -- Steve > > > > I added "__string_len()" and "__assign_str_len()". You use them just like > > __string() and __assign_str() but add a max length that you want to use > > (although, it will always allocate "len" regardless if the string is > > smaller). Then use __get_str() just like you use __string(). > > > > Would something like that work? > > > > -- Steve > >
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h index 27a93ebd1d80..781af519b40c 100644 --- a/fs/nfsd/trace.h +++ b/fs/nfsd/trace.h @@ -400,19 +400,17 @@ TRACE_EVENT(nfsd_dirent, TP_STRUCT__entry( __field(u32, fh_hash) __field(u64, ino) - __field(int, len) - __dynamic_array(unsigned char, name, namlen) + __dynamic_array(char, name, namlen + 1) ), 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); - __assign_str(name, name); + __get_str(name)[namlen] = '\0'; ), - TP_printk("fh_hash=0x%08x ino=%llu name=%.*s", - __entry->fh_hash, __entry->ino, - __entry->len, __get_str(name)) + TP_printk("fh_hash=0x%08x ino=%llu name=%s", + __entry->fh_hash, __entry->ino, __get_str(name) + ) ) #include "state.h"
Since commit 9a6944fee68e ("tracing: Add a verifier to check string pointers for trace events"), which was merged in v5.13-rc1, TP_printk() no longer tacitly supports the "%.*s" format specifier. Note that __string() and __assign_str() cannot be used for non-NUL-terminated C strings because they perform a strlen() on the string that is to be copied. Instead, memcpy the whole file name into the record, but display just the part up to the first NUL. In almost every case that will show the whole file name. Reported-by: David Wysochanski <dwysocha@redhat.com> Fixes: 6019ce0742ca ("NFSD: Add a tracepoint to record directory entry encoding") Signed-off-by: Chuck Lever <chuck.lever@oracle.com> Cc: Steven Rostedt <rostedt@goodmis.org> --- fs/nfsd/trace.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)