From: "Gaëtan Rivet" <gaetan.rivet@6wind.com>
To: Matan Azrad <matan@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [PATCH v2] net/failsafe: fix parameters parsing
Date: Mon, 21 Aug 2017 15:25:33 +0200 [thread overview]
Message-ID: <20170821132533.GW8124@bidouze.vm.6wind.com> (raw)
In-Reply-To: <DB6PR0502MB30484BCF0B71ACB5384D6910D2870@DB6PR0502MB3048.eurprd05.prod.outlook.com>
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 <matan@mellanox.com>
> > 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 <matan@mellanox.com>
> > > ---
> > > 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
next prev parent reply other threads:[~2017-08-21 13:25 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-17 14:19 [dpdk-stable] [PATCH] " Matan Azrad
2017-08-17 15:25 ` Gaëtan Rivet
2017-08-17 15:54 ` Matan Azrad
2017-08-17 16:24 ` Gaëtan Rivet
2017-08-17 18:52 ` Matan Azrad
2017-08-18 9:03 ` Gaëtan Rivet
2017-08-19 21:24 ` Matan Azrad
2017-08-19 22:07 ` [dpdk-stable] [PATCH v2] " Matan Azrad
2017-08-21 9:34 ` Gaëtan Rivet
2017-08-21 12:02 ` Matan Azrad
2017-08-21 13:25 ` Gaëtan Rivet [this message]
2017-08-27 7:23 ` [dpdk-stable] [PATCH v3] " Matan Azrad
2017-08-28 7:52 ` Gaëtan Rivet
2017-08-29 17:05 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170821132533.GW8124@bidouze.vm.6wind.com \
--to=gaetan.rivet@6wind.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).