| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Dec 2014 14:39:54 +0000
From: Dave P Martin <Dave.Martin@....com>
To: Javi Merino <javi.merino@....com>
Cc: "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Punit Agrawal <Punit.Agrawal@....com>,
"broonie@...nel.org" <broonie@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>
Subject: Re: [RFC PATCH v6 1/9] tracing: Add array printing helpers
On Fri, Dec 05, 2014 at 07:04:12PM +0000, Javi Merino wrote:
> From: Dave Martin <Dave.Martin@....com>
>
> If a trace event contains an array, there is currently no standard
> way to format this for text output. Drivers are currently hacking
> around this by a) local hacks that use the trace_seq functionailty
> directly, or b) just not printing that information. For fixed size
> arrays, formatting of the elements can be open-coded, but this gets
> cumbersome for arrays of non-trivial size.
>
> These approaches result in non-standard content of the event format
> description delivered to userspace, so userland tools needs to be
> taught to understand and parse each array printing method
> individually.
>
> This patch implements common __print_<type>_array() helpers that
> tracepoint implementations can use instead of reinventing them. A
> simple C-style syntax is used to delimit the array and its elements
> {like,this}.
>
> So that the helpers can be used with large static arrays as well as
> dynamic arrays, they take a pointer and element count: they can be
> used with __get_dynamic_array() for use with dynamic arrays.
>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Signed-off-by: Dave Martin <Dave.Martin@....com>
> ---
> include/linux/ftrace_event.h | 9 ++++++++
> include/trace/ftrace.h | 17 +++++++++++++++
> kernel/trace/trace_output.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 77 insertions(+)
>
> diff --git a/include/linux/ftrace_event.h b/include/linux/ftrace_event.h
> index 28672e87e910..415afc53fa51 100644
> --- a/include/linux/ftrace_event.h
> +++ b/include/linux/ftrace_event.h
> @@ -44,6 +44,15 @@ const char *ftrace_print_bitmask_seq(struct trace_seq *p, void *bitmask_ptr,
> const char *ftrace_print_hex_seq(struct trace_seq *p,
> const unsigned char *buf, int len);
>
> +const char *ftrace_print_u8_array_seq(struct trace_seq *p,
> + const u8 *buf, int count);
> +const char *ftrace_print_u16_array_seq(struct trace_seq *p,
> + const u16 *buf, int count);
> +const char *ftrace_print_u32_array_seq(struct trace_seq *p,
> + const u32 *buf, int count);
> +const char *ftrace_print_u64_array_seq(struct trace_seq *p,
> + const u64 *buf, int count);
> +
> struct trace_iterator;
> struct trace_event;
>
> diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
> index 26b4f2e13275..15bc5d417aea 100644
> --- a/include/trace/ftrace.h
> +++ b/include/trace/ftrace.h
> @@ -263,6 +263,19 @@
> #undef __print_hex
> #define __print_hex(buf, buf_len) ftrace_print_hex_seq(p, buf, buf_len)
>
> +#undef __print_u8_array
> +#define __print_u8_array(array, count) \
> + ftrace_print_u8_array_seq(p, array, count)
> +#undef __print_u16_array
> +#define __print_u16_array(array, count) \
> + ftrace_print_u16_array_seq(p, array, count)
> +#undef __print_u32_array
> +#define __print_u32_array(array, count) \
> + ftrace_print_u32_array_seq(p, array, count)
> +#undef __print_u64_array
> +#define __print_u64_array(array, count) \
> + ftrace_print_u64_array_seq(p, array, count)
> +
> #undef DECLARE_EVENT_CLASS
> #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print) \
> static notrace enum print_line_t \
> @@ -676,6 +689,10 @@ static inline void ftrace_test_probe_##call(void) \
> #undef __get_dynamic_array_len
> #undef __get_str
> #undef __get_bitmask
> +#undef __print_u8_array
> +#undef __print_u16_array
> +#undef __print_u32_array
> +#undef __print_u64_array
>
> #undef TP_printk
> #define TP_printk(fmt, args...) "\"" fmt "\", " __stringify(args)
> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index c6977d5a9b12..4a6ee61f30b3 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c
> @@ -186,6 +186,57 @@ ftrace_print_hex_seq(struct trace_seq *p, const unsigned char *buf, int buf_len)
> }
> EXPORT_SYMBOL(ftrace_print_hex_seq);
>
> +static const char *
> +ftrace_print_array_seq(struct trace_seq *p, const void *buf, int buf_len,
> + bool (*iterator)(struct trace_seq *p, const char *prefix,
> + const void **buf, int *buf_len))
> +{
> + const char *ret = trace_seq_buffer_ptr(p);
> + const char *prefix = "";
> +
> + trace_seq_putc(p, '{');
> +
> + while (iterator(p, prefix, &buf, &buf_len))
> + prefix = ",";
Makes sense.
> +
> + trace_seq_putc(p, '}');
> + trace_seq_putc(p, 0);
> +
> + return ret;
> +}
> +
> +#define DEFINE_PRINT_ARRAY(type, printk_type, format) \
> +static bool \
> +ftrace_print_array_iterator_##type(struct trace_seq *p, const char *prefix, \
> + const void **buf, int *buf_len) \
> +{ \
> + const type *__src = *buf; \
> + \
> + if (*buf_len < sizeof(*__src)) \
> + return false; \
> + \
> + trace_seq_printf(p, "%s" format, prefix, (printk_type)*__src++); \
> + \
> + *buf = __src; \
> + *buf_len -= sizeof(*__src); \
> + \
> + return true; \
> +} \
> + \
> +const char *ftrace_print_##type##_array_seq( \
> + struct trace_seq *p, const type *buf, int count) \
> +{ \
> + return ftrace_print_array_seq(p, buf, (count) * sizeof(type), \
> + ftrace_print_array_iterator_##type); \
> +} \
> + \
> +EXPORT_SYMBOL(ftrace_print_##type##_array_seq)
> +
> +DEFINE_PRINT_ARRAY(u8, unsigned int, "0x%x");
> +DEFINE_PRINT_ARRAY(u16, unsigned int, "0x%x");
> +DEFINE_PRINT_ARRAY(u32, unsigned int, "0x%x");
> +DEFINE_PRINT_ARRAY(u64, unsigned long long, "0x%llx");
(Responding to Steven's original comment on the choice of types here:)
Steven Rostedt wrote:
> > +DEFINE_PRINT_ARRAY(u8, unsigned int, "0x%x")
>
> Why not "unsigned char"?
>
> > +DEFINE_PRINT_ARRAY(u16, unsigned int, "0x%x")
>
> Why not "unsigned short"?
[...]
The original reason was to work around inconsistent definitions of u32
(unsigned int versus unsigned long) on different arches, so there was
no way to write the format string that wouldn't trigger GCC warnings
on some architectures. The kernel doesn't seem to provide the nasty
C99 PRIxNN macros for now.
Now that all arches seem to use asm-generic to define the u32 type, it
looks like everyone uses the same definition, so this is probably not
needed any more.
I suggest we remove the printk_type argument completely and just use
the underlying uNN type -- but build-testing on a mix of few 32-bit and
64-bit arches would be recommended, just to be on the safe side.
Is there already precedent for this? It seems likely, but I haven't
searched thoroughly.
Cheers
---Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists