Use of uninitialized memory in rate_control_pid_alloc()

Use of uninitialized memory in rate_control_pid_alloc()

[Vegard Nossum]

Hi,

kmemcheck found this in next-20080704:

This patch:

commit 1946b74ce03c4edecabde80d027da00a7eab56ca
Author: Mattias Nissler <mattias.nissler@gmx.de>
Date:   Thu Dec 20 13:27:26 2007 +0100

    rc80211-pid: export tuning parameters through debugfs

contained this hunk (net/mac80211/rc80211_pid_algo.c):

@@ -363,10 +375,10 @@ static void *rate_control_pid_alloc(struct ieee80211_local
        for (i = 0; i < mode->num_rates; i++) {
                rinfo[i].index = i;
                rinfo[i].rev_index = i;
-               if (RC_PID_FAST_START)
+               if (pinfo->fast_start)
                        rinfo[i].diff = 0;
                else
-                       rinfo[i].diff = i * RC_PID_NORM_OFFSET;
+                       rinfo[i].diff = i * pinfo->norm_offset;
        }
        for (i = 1; i < mode->num_rates; i++) {
                s = 0;

which is obviously wrong, since "pinfo" is allocated just above and
has never been initialized.

It seems that this is present (unfixed) in mainline as well.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

[PATCH] rc80211_pid: Fix fast_start parameter handling

[Mattias Nissler]

This removes the fast_start parameter from the rc_pid parameters information
and instead uses the parameter macro when initializing the rc_pid state. Since the
parameter is only used on initialization, there is no point of making exporting
it via debugfs. This also fixes uninitialized memory references to the
fast_start and norm_offset parameters.

Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
---

This should fix the problem. I think this should go into mainline ASAP.

Stefano, any comments?



 net/mac80211/rc80211_pid.h      |    5 -----
 net/mac80211/rc80211_pid_algo.c |   31 +++++++++++++------------------
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/net/mac80211/rc80211_pid.h b/net/mac80211/rc80211_pid.h
index 04afc13..4ea7b97 100644
--- a/net/mac80211/rc80211_pid.h
+++ b/net/mac80211/rc80211_pid.h
@@ -141,7 +141,6 @@ struct rc_pid_events_file_info {
  *	rate behaviour values (lower means we should trust more what we learnt
  *	about behaviour of rates, higher means we should trust more the natural
  *	ordering of rates)
- * @fast_start: if Y, push high rates right after initialization
  */
 struct rc_pid_debugfs_entries {
 	struct dentry *dir;
@@ -154,7 +153,6 @@ struct rc_pid_debugfs_entries {
 	struct dentry *sharpen_factor;
 	struct dentry *sharpen_duration;
 	struct dentry *norm_offset;
-	struct dentry *fast_start;
 };
 
 void rate_control_pid_event_tx_status(struct rc_pid_event_buffer *buf,
@@ -267,9 +265,6 @@ struct rc_pid_info {
 	/* Normalization offset. */
 	unsigned int norm_offset;
 
-	/* Fast starst parameter. */
-	unsigned int fast_start;
-
 	/* Rates information. */
 	struct rc_pid_rateinfo *rinfo;
 
diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index a849b74..bcd27c1 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -398,13 +398,25 @@ static void *rate_control_pid_alloc(struct ieee80211_local *local)
 		return NULL;
 	}
 
+	pinfo->target = RC_PID_TARGET_PF;
+	pinfo->sampling_period = RC_PID_INTERVAL;
+	pinfo->coeff_p = RC_PID_COEFF_P;
+	pinfo->coeff_i = RC_PID_COEFF_I;
+	pinfo->coeff_d = RC_PID_COEFF_D;
+	pinfo->smoothing_shift = RC_PID_SMOOTHING_SHIFT;
+	pinfo->sharpen_factor = RC_PID_SHARPENING_FACTOR;
+	pinfo->sharpen_duration = RC_PID_SHARPENING_DURATION;
+	pinfo->norm_offset = RC_PID_NORM_OFFSET;
+	pinfo->rinfo = rinfo;
+	pinfo->oldrate = 0;
+
 	/* Sort the rates. This is optimized for the most common case (i.e.
 	 * almost-sorted CCK+OFDM rates). Kind of bubble-sort with reversed
 	 * mapping too. */
 	for (i = 0; i < sband->n_bitrates; i++) {
 		rinfo[i].index = i;
 		rinfo[i].rev_index = i;
-		if (pinfo->fast_start)
+		if (RC_PID_FAST_START)
 			rinfo[i].diff = 0;
 		else
 			rinfo[i].diff = i * pinfo->norm_offset;
@@ -425,19 +437,6 @@ static void *rate_control_pid_alloc(struct ieee80211_local *local)
 			break;
 	}
 
-	pinfo->target = RC_PID_TARGET_PF;
-	pinfo->sampling_period = RC_PID_INTERVAL;
-	pinfo->coeff_p = RC_PID_COEFF_P;
-	pinfo->coeff_i = RC_PID_COEFF_I;
-	pinfo->coeff_d = RC_PID_COEFF_D;
-	pinfo->smoothing_shift = RC_PID_SMOOTHING_SHIFT;
-	pinfo->sharpen_factor = RC_PID_SHARPENING_FACTOR;
-	pinfo->sharpen_duration = RC_PID_SHARPENING_DURATION;
-	pinfo->norm_offset = RC_PID_NORM_OFFSET;
-	pinfo->fast_start = RC_PID_FAST_START;
-	pinfo->rinfo = rinfo;
-	pinfo->oldrate = 0;
-
 #ifdef CONFIG_MAC80211_DEBUGFS
 	de = &pinfo->dentries;
 	de->dir = debugfs_create_dir("rc80211_pid",
@@ -465,9 +464,6 @@ static void *rate_control_pid_alloc(struct ieee80211_local *local)
 	de->norm_offset = debugfs_create_u32("norm_offset",
 					     S_IRUSR | S_IWUSR, de->dir,
 					     &pinfo->norm_offset);
-	de->fast_start = debugfs_create_bool("fast_start",
-					     S_IRUSR | S_IWUSR, de->dir,
-					     &pinfo->fast_start);
 #endif
 
 	return pinfo;
@@ -479,7 +475,6 @@ static void rate_control_pid_free(void *priv)
 #ifdef CONFIG_MAC80211_DEBUGFS
 	struct rc_pid_debugfs_entries *de = &pinfo->dentries;
 
-	debugfs_remove(de->fast_start);
 	debugfs_remove(de->norm_offset);
 	debugfs_remove(de->sharpen_duration);
 	debugfs_remove(de->sharpen_factor);
-- 
1.5.6.1

Re: [PATCH] rc80211_pid: Fix fast_start parameter handling

[Vegard Nossum]

Hi,

On Mon, Jul 7, 2008 at 10:23 PM, Mattias Nissler <mattias.nissler@gmx.de> wrote:
> This removes the fast_start parameter from the rc_pid parameters information
> and instead uses the parameter macro when initializing the rc_pid state. Since the
> parameter is only used on initialization, there is no point of making exporting
> it via debugfs. This also fixes uninitialized memory references to the
> fast_start and norm_offset parameters.
>
> Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
> ---
>
> This should fix the problem. I think this should go into mainline ASAP.

Was this code actually causing some problem/regression? If so, can we
please have a reference to bugzilla/prior conversations in the patch
description?

Was the reason for the problem known before I posted the "use of
uninitialized memory" e-mail earlier today? If no, can you please
sneak in a little reference to kmemcheck there in the patch
description? The acknowledgement is sorely needed :-)

Thanks!


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH] rc80211_pid: Fix fast_start parameter handling

[Mattias Nissler]

On Mon, 2008-07-07 at 22:31 +0200, Vegard Nossum wrote:
> Hi,
> 
> On Mon, Jul 7, 2008 at 10:23 PM, Mattias Nissler <mattias.nissler@gmx.de> wrote:
> > This removes the fast_start parameter from the rc_pid parameters information
> > and instead uses the parameter macro when initializing the rc_pid state. Since the
> > parameter is only used on initialization, there is no point of making exporting
> > it via debugfs. This also fixes uninitialized memory references to the
> > fast_start and norm_offset parameters.
> >
> > Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
> > ---
> >
> > This should fix the problem. I think this should go into mainline ASAP.
> 
> Was this code actually causing some problem/regression? If so, can we
> please have a reference to bugzilla/prior conversations in the patch
> description?

I think the problem probably didn't cause any major problem at all. At
most, the rate scaling didn't work properly. Maybe the algorithm would
even recover from the incorrect initialization (not too sure about this
one, better ask Stefano about the rate sorting implications). And no, I
have never seen any problem reports that relate to this bug. I wouldn't
call it a regression either, since this is exactly the code that went
mainline with the first version of rc80211_pid.

> 
> Was the reason for the problem known before I posted the "use of
> uninitialized memory" e-mail earlier today? If no, can you please
> sneak in a little reference to kmemcheck there in the patch
> description? The acknowledgement is sorely needed :-)

It was unknown before. Just a sec, I'll post new version of the patch
that mentions kmemcheck ;-)

Mattias

Re: [PATCH v2] rc80211_pid: Fix fast_start parameter handling

[Mattias Nissler]

This removes the fast_start parameter from the rc_pid parameters information
and instead uses the parameter macro when initializing the rc_pid state. Since
the parameter is only used on initialization, there is no point of making
exporting it via debugfs. This also fixes uninitialized memory references to
the fast_start and norm_offset parameters detected by the kmemcheck utility.
Thanks to Vegard Nossum for reporting the bug.

Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
---

Updated patch description. Any more comments? Stefano? John?

Mattias


 net/mac80211/rc80211_pid.h      |    5 -----
 net/mac80211/rc80211_pid_algo.c |   31 +++++++++++++------------------
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/net/mac80211/rc80211_pid.h b/net/mac80211/rc80211_pid.h
index 04afc13..4ea7b97 100644
--- a/net/mac80211/rc80211_pid.h
+++ b/net/mac80211/rc80211_pid.h
@@ -141,7 +141,6 @@ struct rc_pid_events_file_info {
  *	rate behaviour values (lower means we should trust more what we learnt
  *	about behaviour of rates, higher means we should trust more the natural
  *	ordering of rates)
- * @fast_start: if Y, push high rates right after initialization
  */
 struct rc_pid_debugfs_entries {
 	struct dentry *dir;
@@ -154,7 +153,6 @@ struct rc_pid_debugfs_entries {
 	struct dentry *sharpen_factor;
 	struct dentry *sharpen_duration;
 	struct dentry *norm_offset;
-	struct dentry *fast_start;
 };
 
 void rate_control_pid_event_tx_status(struct rc_pid_event_buffer *buf,
@@ -267,9 +265,6 @@ struct rc_pid_info {
 	/* Normalization offset. */
 	unsigned int norm_offset;
 
-	/* Fast starst parameter. */
-	unsigned int fast_start;
-
 	/* Rates information. */
 	struct rc_pid_rateinfo *rinfo;
 
diff --git a/net/mac80211/rc80211_pid_algo.c b/net/mac80211/rc80211_pid_algo.c
index a849b74..bcd27c1 100644
--- a/net/mac80211/rc80211_pid_algo.c
+++ b/net/mac80211/rc80211_pid_algo.c
@@ -398,13 +398,25 @@ static void *rate_control_pid_alloc(struct ieee80211_local *local)
 		return NULL;
 	}
 
+	pinfo->target = RC_PID_TARGET_PF;
+	pinfo->sampling_period = RC_PID_INTERVAL;
+	pinfo->coeff_p = RC_PID_COEFF_P;
+	pinfo->coeff_i = RC_PID_COEFF_I;
+	pinfo->coeff_d = RC_PID_COEFF_D;
+	pinfo->smoothing_shift = RC_PID_SMOOTHING_SHIFT;
+	pinfo->sharpen_factor = RC_PID_SHARPENING_FACTOR;
+	pinfo->sharpen_duration = RC_PID_SHARPENING_DURATION;
+	pinfo->norm_offset = RC_PID_NORM_OFFSET;
+	pinfo->rinfo = rinfo;
+	pinfo->oldrate = 0;
+
 	/* Sort the rates. This is optimized for the most common case (i.e.
 	 * almost-sorted CCK+OFDM rates). Kind of bubble-sort with reversed
 	 * mapping too. */
 	for (i = 0; i < sband->n_bitrates; i++) {
 		rinfo[i].index = i;
 		rinfo[i].rev_index = i;
-		if (pinfo->fast_start)
+		if (RC_PID_FAST_START)
 			rinfo[i].diff = 0;
 		else
 			rinfo[i].diff = i * pinfo->norm_offset;
@@ -425,19 +437,6 @@ static void *rate_control_pid_alloc(struct ieee80211_local *local)
 			break;
 	}
 
-	pinfo->target = RC_PID_TARGET_PF;
-	pinfo->sampling_period = RC_PID_INTERVAL;
-	pinfo->coeff_p = RC_PID_COEFF_P;
-	pinfo->coeff_i = RC_PID_COEFF_I;
-	pinfo->coeff_d = RC_PID_COEFF_D;
-	pinfo->smoothing_shift = RC_PID_SMOOTHING_SHIFT;
-	pinfo->sharpen_factor = RC_PID_SHARPENING_FACTOR;
-	pinfo->sharpen_duration = RC_PID_SHARPENING_DURATION;
-	pinfo->norm_offset = RC_PID_NORM_OFFSET;
-	pinfo->fast_start = RC_PID_FAST_START;
-	pinfo->rinfo = rinfo;
-	pinfo->oldrate = 0;
-
 #ifdef CONFIG_MAC80211_DEBUGFS
 	de = &pinfo->dentries;
 	de->dir = debugfs_create_dir("rc80211_pid",
@@ -465,9 +464,6 @@ static void *rate_control_pid_alloc(struct ieee80211_local *local)
 	de->norm_offset = debugfs_create_u32("norm_offset",
 					     S_IRUSR | S_IWUSR, de->dir,
 					     &pinfo->norm_offset);
-	de->fast_start = debugfs_create_bool("fast_start",
-					     S_IRUSR | S_IWUSR, de->dir,
-					     &pinfo->fast_start);
 #endif
 
 	return pinfo;
@@ -479,7 +475,6 @@ static void rate_control_pid_free(void *priv)
 #ifdef CONFIG_MAC80211_DEBUGFS
 	struct rc_pid_debugfs_entries *de = &pinfo->dentries;
 
-	debugfs_remove(de->fast_start);
 	debugfs_remove(de->norm_offset);
 	debugfs_remove(de->sharpen_duration);
 	debugfs_remove(de->sharpen_factor);
-- 
1.5.6.1

Re: [PATCH v2] rc80211_pid: Fix fast_start parameter handling

[Vegard Nossum]

On Mon, Jul 7, 2008 at 11:08 PM, Mattias Nissler <mattias.nissler@gmx.de> wrote:
> This removes the fast_start parameter from the rc_pid parameters information
> and instead uses the parameter macro when initializing the rc_pid state. Since
> the parameter is only used on initialization, there is no point of making
> exporting it via debugfs. This also fixes uninitialized memory references to
> the fast_start and norm_offset parameters detected by the kmemcheck utility.
> Thanks to Vegard Nossum for reporting the bug.
>
> Signed-off-by: Mattias Nissler <mattias.nissler@gmx.de>
> ---
>
> Updated patch description. Any more comments? Stefano? John?
>
> Mattias

Hi,

I've tested the patch now. This was my config:

CONFIG_MAC80211_RC_DEFAULT_PID=y
CONFIG_MAC80211_RC_PID=y

and I'm using b43 driver successfully:

CONFIG_B43=y
CONFIG_B43_PCI_AUTOSELECT=y
CONFIG_B43_PCICORE_AUTOSELECT=y
CONFIG_B43_DEBUG=y

This works too:

# cat /debug/ieee80211/phy0/rc80211_pid/*
15
9
15
3
125
0
0
3
14

I guess you may put Tested-by: if this seems enough to verify that the
patch causes no other harm.

Can any of the net people acknowledge the reception of this patch? If
it doesn't make it for 2.6.26, we should at least Cc stable.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH v2] rc80211_pid: Fix fast_start parameter handling

[Ingo Molnar]

* Mattias Nissler <mattias.nissler@gmx.de> wrote:

> [...] This also fixes uninitialized memory references to the 
> fast_start and norm_offset parameters detected by the kmemcheck 
> utility.

cool :-)

	Ingo