From mboxrd@z Thu Jan 1 00:00:00 1970 From: simon.marchi@ericsson.com (Simon Marchi) Date: Tue, 4 Oct 2016 15:48:34 -0400 Subject: [lttng-dev] [PATCH lttng-modules] Add support for i2c tracepoints In-Reply-To: <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> References: <20161003182115.30428-1-simon.marchi@ericsson.com> <93496556.44682.1475533300165.JavaMail.zimbra@efficios.com> <1386914197.45790.1475602673694.JavaMail.zimbra@efficios.com> Message-ID: On 16-10-04 01:37 PM, Mathieu Desnoyers wrote: > Perhaps we could rename the option to "trace_sensitive_payload" ? Of all those: - trace_sensitive_data - trace_sensitive_payload - allow_sensitive_payload_extraction - extract_sensitive_payload I prefer "extract_sensitive_payload". But it's your call, just tell me which one you prefer. > I think we should keep it at the probe level, like you do in your prototype. > >> >> >> From 69aa1c3d73b5b0dbf031b90c6e38d4b2a0ad8699 Mon Sep 17 00:00:00 2001 >> From: Simon Marchi >> Date: Tue, 4 Oct 2016 11:38:19 -0400 >> Subject: [PATCH] Provide knob to control tracing of i2c buffer contents >> >> --- >> instrumentation/events/lttng-module/i2c.h | 6 ++++-- >> probes/lttng-probe-i2c.c | 7 +++++++ >> 2 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/instrumentation/events/lttng-module/i2c.h >> b/instrumentation/events/lttng-module/i2c.h >> index 68ce80d..1ede276 100644 >> --- a/instrumentation/events/lttng-module/i2c.h >> +++ b/instrumentation/events/lttng-module/i2c.h >> @@ -26,7 +26,8 @@ LTTNG_TRACEPOINT_EVENT(i2c_write, >> ctf_integer(__u16, addr, msg->addr) >> ctf_integer(__u16, flags, msg->flags) >> ctf_integer(__u16, len, msg->len) >> - ctf_sequence_hex(__u8, buf, msg->buf, __u16, msg->len) >> + ctf_sequence_hex(__u8, buf, trace_sensitive_data ? msg->buf : NULL, >> + __u16, trace_sensitive_data ? msg->len : 0) > > Since "trace_sensitive_data" can be changed at runtime concurrently > with tracing, we should load it with LOAD_ONCE(), keep it in a local > variable (TRACEPOINT_EVENT_CODE, TP_locvar and TP_code), and use that > in as argument. Otherwise the compiler can perform two fetch of that > variable, and its value may change in between, thus leading to a > NULL pointer dereference (OOPS). Ahh good catch. I'll look up those things you mentioned, but do you have a usage pattern in mind that we could use here? Where would the local variable be? I know of gcc's statement expression, we could use that, but do you have something else in mind? >> ) >> ) >> >> @@ -63,7 +64,8 @@ LTTNG_TRACEPOINT_EVENT(i2c_reply, >> ctf_integer(__u16, addr, msg->addr) >> ctf_integer(__u16, flags, msg->flags) >> ctf_integer(__u16, len, msg->len) >> - ctf_sequence_hex(__u8, buf, msg->buf, __u16, msg->len) >> + ctf_sequence_hex(__u8, buf, trace_sensitive_data ? msg->buf : NULL, >> + __u16, trace_sensitive_data ? msg->len : 0) >> ) >> ) >> > > Same here. > > Can you also fold it with your i2c patch ? I'll send a v2 of the original patch once it's clear how to do the load once thingy. Thanks! Simon