From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4F28AA0471 for ; Tue, 16 Jul 2019 16:58:54 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1078A37B0; Tue, 16 Jul 2019 16:58:51 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id BDB0D37B0 for ; Tue, 16 Jul 2019 16:58:48 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Jul 2019 07:58:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,498,1557212400"; d="scan'208";a="187356364" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga001.fm.intel.com with ESMTP; 16 Jul 2019 07:58:47 -0700 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 16 Jul 2019 07:58:47 -0700 Received: from fmsmsx117.amr.corp.intel.com ([169.254.3.206]) by FMSMSX151.amr.corp.intel.com ([169.254.7.106]) with mapi id 14.03.0439.000; Tue, 16 Jul 2019 07:58:47 -0700 From: "Carrillo, Erik G" To: "Richardson, Bruce" CC: 'Stephen Hemminger' , "'thomas@monjalon.net'" , "'dev@dpdk.org'" Thread-Topic: [dpdk-dev] [PATCH 1/2] timer: fix null pointer dereference Thread-Index: AQHVOyON7FzSx+6Ovkut0ZzoYIKzHKbMSJ6A//+OPaCAADqeUIABTzMA///2X3A= Date: Tue, 16 Jul 2019 14:58:46 +0000 Message-ID: References: <1563205172-352-1-git-send-email-erik.g.carrillo@intel.com> <1563205172-352-2-git-send-email-erik.g.carrillo@intel.com> <20190715084841.4f6baebf@hermes.lan> <20190716083102.GA561@bricha3-MOBL.ger.corp.intel.com> In-Reply-To: <20190716083102.GA561@bricha3-MOBL.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.1.200.108] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/2] timer: fix null pointer dereference 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Bruce Richardson > Sent: Tuesday, July 16, 2019 3:31 AM > To: Carrillo, Erik G > Cc: 'Stephen Hemminger' ; > 'thomas@monjalon.net' ; 'dev@dpdk.org' > > Subject: Re: [dpdk-dev] [PATCH 1/2] timer: fix null pointer dereference >=20 > On Mon, Jul 15, 2019 at 07:48:09PM +0000, Carrillo, Erik G wrote: > > > -----Original Message----- > > > From: Carrillo, Erik G > > > Sent: Monday, July 15, 2019 11:04 AM > > > To: Stephen Hemminger > > > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org > > > Subject: RE: [dpdk-dev] [PATCH 1/2] timer: fix null pointer > > > dereference > > > > > > Hi Stephen, > > > > > > > -----Original Message----- > > > > From: Stephen Hemminger > > > > Sent: Monday, July 15, 2019 10:49 AM > > > > To: Carrillo, Erik G > > > > Cc: thomas@monjalon.net; dev@dpdk.org; stable@dpdk.org > > > > Subject: Re: [dpdk-dev] [PATCH 1/2] timer: fix null pointer > > > > dereference > > > > > > > > On Mon, 15 Jul 2019 10:39:31 -0500 Erik Gabriel Carrillo > > > > wrote: > > > > > > > > > If the timer subsystem is not initialized before > > > > > rte_timer_manage (for > > > > > example) is invoked, a pointer to a shared hugepage memory > > > > > region will still be null and dereferenced when it is checked > > > > > for validity; handle this case. > > > > > > > > > > Fixes: c0749f7096c7 ("timer: allow management in shared memory") > > > > > Cc: stable@dpdk.org > > > > > > > > > > Signed-off-by: Erik Gabriel Carrillo > > > > > > > > I have mixed feelings about this patch. > > > > Any calls to rte_timer before rte_timer_subsystem_init is not a val= id > usage. > > > > Better to kill the application. > > > > > > Ok, that sounds like a better approach. I'll update the patch and > resubmit. > > > > > > > I added a call to rte_exit() in the timer_data_valid() function for the= case > where the library is uninitialized, but checkpatches.sh issues the follow= ing > warning: > > > > "Warning in /lib/librte_timer/rte_timer.c: > > Using rte_panic/rte_exit" > > > > According to the comments in the script, we should refrain from new > additions of rte_panic() and rte_exit() in the lib subtree. In light of= this, > should we still proceed with this approach? It does seem like it would b= e > useful. > > >=20 > I don't think we should ever put panics or exits in our library code, so = I think > the immediate choices are to either leave things as-is and allow app to c= rash > for invalid use, or else catch the error and return a suitable error code= to the > user. I think I'd prefer the latter. >=20 In that case, I'd like to keep the current patch out for consideration. It= detects the error and enables the library APIs to return an error code to = the user. > However, given that the error condition is not having the timer subsystem > initialized, is there the possibility of a third option to just go and in= itialize > before continuing in the timer_manage() function? It seems like this could work, but I'd like to hold off for more investigat= ion. Thanks, Erik