From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f171.google.com (mail-wr0-f171.google.com [209.85.128.171]) by dpdk.org (Postfix) with ESMTP id D93567CD2 for ; Mon, 21 Aug 2017 15:25:43 +0200 (CEST) Received: by mail-wr0-f171.google.com with SMTP id p8so34972279wrf.5 for ; Mon, 21 Aug 2017 06:25:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=Ir28e9urrLBVYQpwUhYHxxmfn1i4JuUfR9zKg3MKgGs=; b=ddWbUt3Pi4B6VQB9tumv54/PcJrW/ASsBPINFbVskz7LarmMYy8aekOSZ1g6V2ZswY GoDBA2/LlILhYUPn9lM/54ui7/x3UcK2dhDdbrd/hqFN/140aqHU3W6zZmthGxhcV7uV LN1smh43RzROe2Ec7+0vhXhmqLYjOV5MhvqBeeysoKMCij9JAhaThydaLHZkUUgWLtKp mUlPJXKB6KrOno5ftkPfTFNsaY/pouxqHHO/SklJ2M8Noedt0rqJuqwx4moCJSk+MZxw Lv94klDPATBaWAS5+UFBQ5tC8ILc141JMMpzvsWiuvdjLdb8rJdZWuYj/s0T9y8MjLDB WLGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=Ir28e9urrLBVYQpwUhYHxxmfn1i4JuUfR9zKg3MKgGs=; b=DfWKcSO0s0ZmqNYFWtz4RAQHSLwBEI6rw+t1cq2G3P9Byq0rE32JGCOmYJrTsum0x4 MxxFbjdEHAMwV+9cRO4mVxAfnk3HNqBFHU6wm9WsBZjXYrO+EEUK0NOOrn7QXVhgoglB xK8FGH2S4gHjBOY1IFEtarEjUWN0haBiTogO5HXmpTU3opf6yyKjVLWo83iQsE4lVAxb 15MPVJAXseo852t4tS5vopNcOc5K5/uzZtmQGrKEouT7op3qw0J6orhHJ/av+HYfPInp W/2LLCtdldEK54wQrjWr00s/F+z2P4UuFKG7LfOyIFuxT0+L9kvVdo1hcVhHeXO9Kch7 s+kA== X-Gm-Message-State: AHYfb5i1Dms6y1hYAnY0IJLkFfEhhJl/PLQuH+6i+DUomwV9He0O4LQY aqz5pH6YL2EbL9s+ X-Received: by 10.223.176.162 with SMTP id i31mr12171889wra.221.1503321943426; Mon, 21 Aug 2017 06:25:43 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id e2sm8347803wrd.60.2017.08.21.06.25.42 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 21 Aug 2017 06:25:42 -0700 (PDT) Date: Mon, 21 Aug 2017 15:25:33 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: "dev@dpdk.org" , "stable@dpdk.org" Message-ID: <20170821132533.GW8124@bidouze.vm.6wind.com> References: <1503180443-9687-1-git-send-email-matan@mellanox.com> <20170821093426.GV8124@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-stable] [PATCH v2] net/failsafe: fix parameters parsing X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 21 Aug 2017 13:25:44 -0000 On Mon, Aug 21, 2017 at 12:02:44PM +0000, Matan Azrad wrote: > Hi Gaetan > > > -----Original Message----- > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com] > > Sent: Monday, August 21, 2017 12:34 PM > > To: Matan Azrad > > Cc: dev@dpdk.org; stable@dpdk.org > > Subject: Re: [PATCH v2] net/failsafe: fix parameters parsing > > > > Hi Matan, > > > > On Sun, Aug 20, 2017 at 01:07:23AM +0300, Matan Azrad wrote: > > > The corrupted code used wrongly snprintf return value as the number of > > > characters actually copied, in spite of the meanning is the number of > > > characters which would be generated for the given input. > > > > > > It caused to remain zerod bytes between the failsafe command line non > > > sub device parameters indicates end of string. > > > > > > Hence, when rte_kvargs_parse tried to parse all parameters, it got end > > > of string after the first one and the others weren't parsed. > > > > > > So, if the mac parameters was the first in command line it was taken > > > while hotplug_poll was left default, and vice versa. > > > > > > The fix updates the buffer index by dedicated variable contains the > > > copy size, by the way validates the last comma removing. > > > > > > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Matan Azrad > > > --- > > > drivers/net/failsafe/failsafe_args.c | 13 ++++++++++--- > > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/failsafe/failsafe_args.c > > > b/drivers/net/failsafe/failsafe_args.c > > > index 1f22416..2a5760a 100644 > > > --- a/drivers/net/failsafe/failsafe_args.c > > > +++ b/drivers/net/failsafe/failsafe_args.c > > > @@ -286,10 +286,14 @@ fs_remove_sub_devices_definition(char > > params[DEVARGS_MAXLEN]) > > > ERROR("Invalid parameter"); > > > return -EINVAL; > > > } > > > - if (params[b] == ',' || params[b] == '\0') > > > - i += snprintf(&buffer[i], b - a + 1, "%s", ¶ms[a]); > > > - if (params[b] == '(') { > > > + if (params[b] == ',' || params[b] == '\0') { > > > + size_t len = b - a + 1; > > > + > > > + snprintf(&buffer[i], len, "%s", ¶ms[a]); > > > > There it should be: > > > > snprintf(&buffer[i], len + 1, "%s", ¶ms[a]); > > > > You right - will be fixed in V3. > > > This is due to the use of snprintf intead of memcpy. It illustrates actually why > > the overhead of using snprintf is worth it, as it would exactly be the situation > > where memcpy would copy the last character as a comma and rely on buffer > > being clean (well, it is, but that's beside the point :). > > > > I don't think so. > We know where is the end of string in this branch - so don't need snprintf for safety. > Actually, it illustrates why in this case we should use memcpy :) > > > If for any reason (performance? Lunacy?) someone wanted to change the > > initialization of buffer (for example, directly working on params instead of > > copying in a temporary buffer first, or simply removing the "= {0};" above > > because he is a maniac), then this chunk of code would become unsafe > > without snprintf. > > > For performance\latency I think this one will replace snprintf too. > Even for "={0}" removing the branch in the end of function I added(i>0) covers it. > This is not a performance-critical section. The latency is irrelevant because this code is executed once, before any packet processing has started. The only focus is on correctness and simplicity. > > > + i += len; > > > + } else if (params[b] == '(') { > > > size_t start = b; > > > + > > > b += closing_paren(¶ms[b]); > > > if (b == start) > > > return -EINVAL; > > > @@ -300,6 +304,9 @@ fs_remove_sub_devices_definition(char > > params[DEVARGS_MAXLEN]) > > > a = b + 1; > > > } > > > out: > > > + if (i > 0) > > > + /* For last comma preventing. */ > > > + buffer[i - 1] = '\0'; > > > > I don't think there is a simple way to keep this in the kvarg branch (that > > would involve precomputing the final length of i and checking that we > > reached it within said branch -- pretty ugly). > > > > You would have had to send a v3 anyway due to the OB1 error above. > > > > Compare with this version: > > > > > > if (params[b] == ',' || params[b] == '\0') { > > size_t param_len = b - a; > > > > if (i > 0) > > param_len += 1; > > snprintf(&buffer[i], param_len + 1, "%s%s", > > i ? "," : "", ¶ms[a]); > > i += param_len; > > } > > > > The only added logic is the `a ? "," : ""` conditional, and it allows to keep all > > changes within the kvarg branch, avoiding scattering output string formatting > > logic. > > > > Please use this instead in your v3. > > But you use these 2 branches per parameter, > I suggest only one branch for any parameters number. > > Take example of 2 non sub device parameters: > You suggest 4 branches and I suggest only 1 to handle the last comma removing. > Same as above: performance is irrelevant at this point. > Is it OK for you to use it as is? > I'm ok with the simplest solution that fixes the bug :) . > > > > > snprintf(params, DEVARGS_MAXLEN, "%s", buffer); > > > return 0; > > > } > > > -- > > > 2.7.4 > > > -- Gaëtan Rivet 6WIND