Re: [PATCH v3] net: Add trace events for all receive exit points

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

----- On Nov 16, 2018, at 10:50 PM, David S. Miller davem@davemloft.net wrote:

> From: Geneviève Bastien <gbastien@versatic.net>
> Date: Tue, 13 Nov 2018 15:13:26 -0500
> 
>> @@ -5222,9 +5228,14 @@ static void netif_receive_skb_list_internal(struct
>> list_head *head)
>>   */
>>  int netif_receive_skb(struct sk_buff *skb)
>>  {
>> +	int ret;
>> +
>>  	trace_netif_receive_skb_entry(skb);
>>  
>> -	return netif_receive_skb_internal(skb);
>> +	ret = netif_receive_skb_internal(skb);
>> +	trace_netif_receive_skb_exit(skb, ret);
> 
> Every time I read this code from now on I'm going to say to myself
> "oh crap, we reference 'skb' after it's potentially freed up"
> 
> I really don't like this.
> 
> I know only the pointer is used, but that pointer can be reallocated
> to another SLAB object, even another SKB, by the time these exit
> tracepoints execute.
> 
> Sorry, I can't really convince myself to apply this now.

Hi David,

Thanks for looking into this patch. You bring a very good point indeed!
Passing a skb pointer that may have been already reallocated to skb_exit
tracepoints is pretty much useless for analysis purposes.

I see two possible solutions:

1) Remove the "skb" argument from the sbk_exit tracepoints completely.
Anyway, I think it's not really needed for analysis purposes because
we can link the "entry" with the associated "exit" using the thread ID
executing those tracepoints.  (Geneviève, would that work for your
analyses ?)

2) Move the skb_exit tracepoints before freeing the skb pointer. My
concern here is that the instrumentation may become much uglier than
the currently proposed patch. (I have not looked at the specifics
though, so I may be wrong.)

Do you have a preference between those two approaches, or perhaps you
have an alternative solution in mind ?

Thanks!

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com