[PATCH net v2] gen_stats.c: Duplicate xstats buffer for later use

[PATCH net v2] gen_stats.c: Duplicate xstats buffer for later use

[Ignacy Gawędzki]

The gnet_stats_copy_app() function gets called, more often than not, with its
second argument a pointer to an automatic variable in the caller's stack.
Therefore, to avoid copying garbage afterwards when calling
gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
memory that gets freed after use.

Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
 net/core/gen_stats.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 0c08062..5ad2fe7 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -32,6 +32,9 @@ gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
 	return 0;
 
 nla_put_failure:
+	kfree(d->xstats);
+	d->xstats = NULL;
+	d->xstats_len = 0;
 	spin_unlock_bh(d->lock);
 	return -1;
 }
@@ -305,7 +308,11 @@ int
 gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
 {
 	if (d->compat_xstats) {
-		d->xstats = st;
+		kfree(d->xstats);
+		d->xstats = kmalloc(len, GFP_ATOMIC);
+		if (!d->xstats)
+			goto kmalloc_failure;
+		memcpy(d->xstats, st, len);
 		d->xstats_len = len;
 	}
 
@@ -313,6 +320,10 @@ gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
 		return gnet_stats_copy(d, TCA_STATS_APP, st, len);
 
 	return 0;
+kmalloc_failure:
+	d->xstats_len = 0;
+	spin_unlock_bh(d->lock);
+	return -1;
 }
 EXPORT_SYMBOL(gnet_stats_copy_app);
 
@@ -345,6 +356,9 @@ gnet_stats_finish_copy(struct gnet_dump *d)
 			return -1;
 	}
 
+	kfree(d->xstats);
+	d->xstats = NULL;
+	d->xstats_len = 0;
 	spin_unlock_bh(d->lock);
 	return 0;
 }
-- 
1.9.1

[PATCH net v3] gen_stats.c: Duplicate xstats buffer for later use

[Ignacy Gawędzki]

The gnet_stats_copy_app() function gets called, more often than not, with its
second argument a pointer to an automatic variable in the caller's stack.
Therefore, to avoid copying garbage afterwards when calling
gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
memory that gets freed after use.

Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
---
 net/core/gen_stats.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 0c08062..c9f1fa8 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -32,6 +32,9 @@ gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
 	return 0;
 
 nla_put_failure:
+	kfree(d->xstats);
+	d->xstats = NULL;
+	d->xstats_len = 0;
 	spin_unlock_bh(d->lock);
 	return -1;
 }
@@ -305,7 +308,10 @@ int
 gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
 {
 	if (d->compat_xstats) {
-		d->xstats = st;
+		kfree(d->xstats);
+		d->xstats = kmemdup(st, len, GFP_ATOMIC);
+		if (!d->xstats)
+			goto kmalloc_failure;
 		d->xstats_len = len;
 	}
 
@@ -313,6 +319,10 @@ gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
 		return gnet_stats_copy(d, TCA_STATS_APP, st, len);
 
 	return 0;
+kmalloc_failure:
+	d->xstats_len = 0;
+	spin_unlock_bh(d->lock);
+	return -1;
 }
 EXPORT_SYMBOL(gnet_stats_copy_app);
 
@@ -345,6 +355,9 @@ gnet_stats_finish_copy(struct gnet_dump *d)
 			return -1;
 	}
 
+	kfree(d->xstats);
+	d->xstats = NULL;
+	d->xstats_len = 0;
 	spin_unlock_bh(d->lock);
 	return 0;
 }
-- 
2.1.0

Re: [PATCH net v3] gen_stats.c: Duplicate xstats buffer for later use

[Cong Wang]

On Thu, Jan 8, 2015 at 5:30 AM, Ignacy Gawędzki
<ignacy.gawedzki@green-communications.fr> wrote:
> The gnet_stats_copy_app() function gets called, more often than not, with its
> second argument a pointer to an automatic variable in the caller's stack.
> Therefore, to avoid copying garbage afterwards when calling
> gnet_stats_finish_copy(), this data is better copied to a dynamically allocated
> memory that gets freed after use.

Good catch!

>
> Signed-off-by: Ignacy Gawędzki <ignacy.gawedzki@green-communications.fr>
> ---
>  net/core/gen_stats.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 0c08062..c9f1fa8 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -32,6 +32,9 @@ gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
>         return 0;
>
>  nla_put_failure:
> +       kfree(d->xstats);
> +       d->xstats = NULL;
> +       d->xstats_len = 0;
>         spin_unlock_bh(d->lock);
>         return -1;
>  }

Although it is not your fault, the API sucks, gnet_stats_copy()
should not need to care about the error path of its caller.
I will cook a patch for it.

> @@ -305,7 +308,10 @@ int
>  gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
>  {
>         if (d->compat_xstats) {
> -               d->xstats = st;
> +               kfree(d->xstats);
> +               d->xstats = kmemdup(st, len, GFP_ATOMIC);
> +               if (!d->xstats)
> +                       goto kmalloc_failure;

Do we really need to call kfree() before kmemdup()?
I don't think gnet_stats_copy_app() is supposed to be called
twice before gnet_stats_copy_finish()?

Thanks.

Re: [PATCH net v3] gen_stats.c: Duplicate xstats buffer for later use

[Ignacy Gawędzki]

On Fri, Jan 09, 2015 at 02:35:41PM -0800, thus spake Cong Wang:
> On Thu, Jan 8, 2015 at 5:30 AM, Ignacy Gawędzki
> > diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> > index 0c08062..c9f1fa8 100644
> > --- a/net/core/gen_stats.c
> > +++ b/net/core/gen_stats.c
> > @@ -32,6 +32,9 @@ gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
> >         return 0;
> >
> >  nla_put_failure:
> > +       kfree(d->xstats);
> > +       d->xstats = NULL;
> > +       d->xstats_len = 0;
> >         spin_unlock_bh(d->lock);
> >         return -1;
> >  }
> 
> Although it is not your fault, the API sucks, gnet_stats_copy()
> should not need to care about the error path of its caller.
> I will cook a patch for it.

That would be great, as I'm certainly not that fluent in kernel code.

> > @@ -305,7 +308,10 @@ int
> >  gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
> >  {
> >         if (d->compat_xstats) {
> > -               d->xstats = st;
> > +               kfree(d->xstats);
> > +               d->xstats = kmemdup(st, len, GFP_ATOMIC);
> > +               if (!d->xstats)
> > +                       goto kmalloc_failure;
> 
> Do we really need to call kfree() before kmemdup()?
> I don't think gnet_stats_copy_app() is supposed to be called
> twice before gnet_stats_copy_finish()?

It's not, but I figured it's safer that way and probably not that expensive
given the frequency with which calls to these functions are performed in
practice.

-- 
Ignacy Gawędzki
R&D Engineer
Green Communications