[lttng-dev] [PATCH lttng-modules] Add support for i2c tracepoints

[Simon Marchi]

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 <simon.marchi at ericsson.com>
>> 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