[PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use

[PATCH net 1/2] 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 | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 0c08062..5770a0e 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -305,7 +305,10 @@ int
 gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
 {
 	if (d->compat_xstats) {
-		d->xstats = st;
+		d->xstats = kmalloc(len, GFP_KERNEL);
+		if (!d->xstats)
+			goto kmalloc_failure;
+		memcpy(d->xstats, st, len);
 		d->xstats_len = len;
 	}
 
@@ -313,6 +316,9 @@ 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:
+	spin_unlock_bh(d->lock);
+	return -1;
 }
 EXPORT_SYMBOL(gnet_stats_copy_app);
 
@@ -340,8 +346,13 @@ gnet_stats_finish_copy(struct gnet_dump *d)
 			return -1;
 
 	if (d->compat_xstats && d->xstats) {
-		if (gnet_stats_copy(d, d->compat_xstats, d->xstats,
-			d->xstats_len) < 0)
+		int	result = gnet_stats_copy(d, d->compat_xstats,
+						 d->xstats, d->xstats_len);
+		kfree(d->xstats);
+		d->xstats = NULL;
+		d->xstats_len = 0;
+
+		if (result < 0)
 			return -1;
 	}
 
-- 
1.9.1

RE: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use

[David Laight]

From: Ignacy Gawedzki
> 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 Gawedzki <ignacy.gawedzki@green-communications.fr>
> ---
>  net/core/gen_stats.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 0c08062..5770a0e 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -305,7 +305,10 @@ int
>  gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
>  {
>  	if (d->compat_xstats) {
> -		d->xstats = st;
> +		d->xstats = kmalloc(len, GFP_KERNEL);
> +		if (!d->xstats)
> +			goto kmalloc_failure;
> +		memcpy(d->xstats, st, len);
>  		d->xstats_len = len;
>  	}
> 
> @@ -313,6 +316,9 @@ 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:
> +	spin_unlock_bh(d->lock);
> +	return -1;
>  }

This rather implies that you are calling kmalloc() with a spin lock help.
If this is valid at all then you should probably specify GPF_ATOMIC.
OTOH it is better to call kmalloc() before acquiring the lock.

The locking itself looks odd - since the corresponding spin_lock_bh()
isn't in the same function.

	David

Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use

['Ignacy Gawedzki']

On Thu, Jan 08, 2015 at 11:05:24AM +0000, thus spake David Laight:
> This rather implies that you are calling kmalloc() with a spin lock help.
> If this is valid at all then you should probably specify GPF_ATOMIC.

Yes, you're right, my mistake.

> OTOH it is better to call kmalloc() before acquiring the lock.

This would certainly be the best solution, but still, it looks pretty hard to
implement this way since the whole sequence of functions is called from
tc_fill_qdisc() and tc_fill_tclass() in net/sched/sch_api.c: first a call to
gnet_stats_start_copy_compat() acquires the lock, then a call to per-qdisc
dump_stats() is performed that itself calls gnet_stats_copy_app() with the
pointer to the automatic structure that needs to be duplicated and finally
gnet_stats_finish_copy() is called that eventually releases the lock.  In the
originaly code, only gnet_stats_copy() can cause a failure and so the release
of the lock is performed there in such a case.

I don't see any easy way of knowing how much to allocate *before* the call to
gnet_stats_start_copy_compat().

> The locking itself looks odd - since the corresponding spin_lock_bh()
> isn't in the same function.

I agree that this doesn't look too good.

For the moment I'll post a corrected patch that uses GPF_ATOMIC.  Then anyone
can come up with a better fix anytime.

Ignacy

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

RE: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use

[David Laight]

From: Ignacy Gawedzki
> 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 Gawedzki <ignacy.gawedzki@green-communications.fr>
> ---
>  net/core/gen_stats.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 0c08062..5770a0e 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -305,7 +305,10 @@ int
>  gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
>  {
>  	if (d->compat_xstats) {
> -		d->xstats = st;
> +		d->xstats = kmalloc(len, GFP_KERNEL);
> +		if (!d->xstats)
> +			goto kmalloc_failure;
> +		memcpy(d->xstats, st, len);
>  		d->xstats_len = len;

Looking at this again, isn't the purpose of the 'void *st' to pass
down a temporary buffer that is 'big enough' ?

	David

Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use

['Ignacy Gawedzki']

On Thu, Jan 08, 2015 at 12:26:17PM +0000, thus spake David Laight:
> Looking at this again, isn't the purpose of the 'void *st' to pass
> down a temporary buffer that is 'big enough' ?

The buffer is large enough at the time of the call, yes.  But the thing is
that in most of the actual callers, it is an automatic variable in the
caller's frame, while the buffer is actually used at a later point when
gnet_stats_finish_copy() is called (when that frame doesn't exist anymore).

I'm about to send a revised version of the patch that also corrects a few
shortcomings in the management of the dynamically allocated buffer w.r.t. the
acquired lock.

Ignacy

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

Re: [PATCH net 1/2] gen_stats.c: Duplicate xstats buffer for later use

[Sergei Shtylyov]

Hello.

On 1/8/2015 1:35 PM, Ignacy Gawędzki 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.

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

> diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
> index 0c08062..5770a0e 100644
> --- a/net/core/gen_stats.c
> +++ b/net/core/gen_stats.c
> @@ -305,7 +305,10 @@ int
>   gnet_stats_copy_app(struct gnet_dump *d, void *st, int len)
>   {
>   	if (d->compat_xstats) {
> -		d->xstats = st;
> +		d->xstats = kmalloc(len, GFP_KERNEL);
> +		if (!d->xstats)
> +			goto kmalloc_failure;
> +		memcpy(d->xstats, st, len);

    Please use kmemdup() instead of kmalloc()/memcpy().

[...]

WBR, Sergei