From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathieu Desnoyers Subject: Re: [PATCH v3] net: Add trace events for all receive exit points Date: Sat, 17 Nov 2018 13:27:29 -0500 (EST) Message-ID: <1286988499.7245.1542479249307.JavaMail.zimbra@efficios.com> References: <20181113201326.5627-1-gbastien@versatic.net> <20181116.195036.1120979831963497428.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: =?utf-8?Q?Genevi=C3=A8ve?= Bastien , netdev , rostedt , Ingo Molnar To: "David S. Miller" Return-path: Received: from mail.efficios.com ([167.114.142.138]:60596 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726071AbeKREpB (ORCPT ); Sat, 17 Nov 2018 23:45:01 -0500 In-Reply-To: <20181116.195036.1120979831963497428.davem@davemloft.net> Sender: netdev-owner@vger.kernel.org List-ID: ----- On Nov 16, 2018, at 10:50 PM, David S. Miller davem@davemloft.net wro= te: > From: Genevi=C3=A8ve Bastien > Date: Tue, 13 Nov 2018 15:13:26 -0500 >=20 >> @@ -5222,9 +5228,14 @@ static void netif_receive_skb_list_internal(struc= t >> list_head *head) >> */ >> int netif_receive_skb(struct sk_buff *skb) >> { >> +=09int ret; >> + >> =09trace_netif_receive_skb_entry(skb); >> =20 >> -=09return netif_receive_skb_internal(skb); >> +=09ret =3D netif_receive_skb_internal(skb); >> +=09trace_netif_receive_skb_exit(skb, ret); >=20 > 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" >=20 > I really don't like this. >=20 > 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. >=20 > 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=C3=A8ve, 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 --=20 Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com