Re: [PATCH lttng-modules] Add support for i2c tracepoints

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- On Oct 4, 2016, at 3:48 PM, Simon Marchi simon.marchi@ericsson.com wrote:

> 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.

Let's go for "extract_sensitive_payload".

> 
>> 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@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?

See http://lttng.org/docs/#doc-lttng-tracepoint-event-code

Note that there are slight changes to TP_code() in 2.8. See
source for details.


> 
>>> 	)
>>> )
>>>
>>> @@ -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.

Allright, thanks!

Mathieu

> 
> Thanks!
> 
> Simon

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev