From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 0F7D15F14; Fri, 27 Apr 2018 15:09:57 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Apr 2018 06:09:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,335,1520924400"; d="scan'208";a="36911631" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.51]) by orsmga008.jf.intel.com with SMTP; 27 Apr 2018 06:09:54 -0700 Received: by (sSMTP sendmail emulation); Fri, 27 Apr 2018 14:09:53 +0100 Date: Fri, 27 Apr 2018 14:09:52 +0100 From: Bruce Richardson To: "Van Haaren, Harry" Cc: Akhil Goyal , "De Lara Guarch, Pablo" , "Zhang, Roy Fan" , "dev@dpdk.org" , "stable@dpdk.org" Message-ID: <20180427130952.GA102852@bricha3-MOBL.ger.corp.intel.com> References: <20180426150950.7568-1-pablo.de.lara.guarch@intel.com> <5e2f48d7-c451-c550-5ddc-70263a278e2f@nxp.com> <5bec1a84-d91f-26cf-1a5d-2909c0906c8f@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after freeing X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Apr 2018 13:09:58 -0000 On Fri, Apr 27, 2018 at 12:37:08PM +0000, Van Haaren, Harry wrote: > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Akhil Goyal > > Sent: Friday, April 27, 2018 12:59 PM > > To: De Lara Guarch, Pablo ; Akhil Goyal > > ; Zhang, Roy Fan > > Cc: dev@dpdk.org; stable@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer after > > freeing > > > > Hi Pablo, > > > > On 4/27/2018 5:06 PM, De Lara Guarch, Pablo wrote: > > > Hi Akhil, > > > > > >> -----Original Message----- > > >> From: Akhil Goyal [mailto:akhil.goyal@nxp.com] > > >> Sent: Friday, April 27, 2018 9:47 AM > > >> To: De Lara Guarch, Pablo ; Zhang, Roy Fan > > >> > > >> Cc: dev@dpdk.org; stable@dpdk.org > > >> Subject: Re: [dpdk-dev] [PATCH 1/2] crypto/scheduler: set null pointer > > after > > >> freeing > > >> > > >> Hi Pablo, > > >> > > >> On 4/26/2018 8:39 PM, Pablo de Lara wrote: > > >>> When freeing memory, pointers should be set to NULL, to avoid memory > > >>> corruption/segmentation faults. > > >> > > >> Shouldn't this be handled in the rte_free itself. A lot of other driver are > > also not > > >> setting null after rte_free. > > >> This would require change at a lot of places if this is not handled in > > rte_free. > > >> > > > > > > The glibc function "free" works the same way. Users are responsible for > > > setting to NULL these pointers (because sometimes, it is not necessary to do > > such thing). > > Yes it is correct but rte_free is custom free API in DPDK which can be > > modified or we can have a safer API rte_free_safe which can set the > > pointer to null. > > > > > > Anyway, in case we still wanted to change it, we would need to pass a > > pointer > > > to a pointer in rte_free, which would imply an API breakage. > > > Actually there is an alternative solution, by creating a macro like so; > > #define rte_free(x) do { > rte_free_(x); /* call the real implementation, now with _ */ > x = NULL; > } while (0) > > This is not an ABI break if symbol versioning is used for rte_free(). > > It is an API change however - not that the calling code has to change, > but rather that the effect of rte_free() changes transparently. > > I'm not sure what the correct thing to do is in this case - just pointing > out that this is another possible solution. > > > > I think if the community agrees, we can add this change may be in next > > releases. > > +1 to discuss this with the community, regardless of the implementation :) > > I really don't think this change is necessary. I think having rte_free behave as libc free is fine. However, if we want to add a new API called rte_free_and_null(void **x), I could be ok with that, though I'd be somewhat dubious of its necessity. Static analysis tools should be able to pick up use-after-free errors, though we may need to provide metadata to the tools in some form indicating that rte_free is a free-ing function. /Bruce