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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1286988499.7245.1542479249307.JavaMail.zimbra@efficios.com>
Date:   Sat, 17 Nov 2018 13:27:29 -0500 (EST)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     "David S. Miller" <davem@...emloft.net>
Cc:     Geneviève Bastien <gbastien@...satic.net>,
        netdev <netdev@...r.kernel.org>, rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v3] net: Add trace events for all receive exit points

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

> From: Geneviève Bastien <gbastien@...satic.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