[dpdk-dev,v2] net/failsafe: fix parameters parsing
Checks
Commit Message
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(-)
Comments
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]);
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 :).
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.
> + 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.
> snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
> return 0;
> }
> --
> 2.7.4
>
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.
> > + 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.
Is it OK for you to use it as is?
>
> > snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
> > return 0;
> > }
> > --
> > 2.7.4
> >
>
> --
> Gaëtan Rivet
> 6WIND
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
> > >
@@ -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]);
+ 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';
snprintf(params, DEVARGS_MAXLEN, "%s", buffer);
return 0;
}