Use of uninitialized memory in rate_control_pid_alloc()

PATCH In review Vegard Nossum 3 participants 7 messages Jul 7, 2008 – Jul 10, 2008

Subsystems (3)

Networking Network drivers mac80211

lore

Share & tools

lore

Vegard Nossum

author

2008-07-07 11:28 UTC #0 lore raw

Hi,

kmemcheck found this in next-20080704:

This patch:

commit 1946b74ce03c4edecabde80d027da00a7eab56ca

Author: Mattias Nissler <[email protected]>
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

Signature · 4 lines

"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

Mattias Nissler

2008-07-07 20:23 UTC #1 lore raw

PATCH 1/1 rc80211_pid: Fix fast_start parameter handling

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 <[email protected]>

---

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(-)

net/mac80211/rc80211_pid.h +0 -5

index 04afc13..4ea7b97 100644
@@ -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;

net/mac80211/rc80211_pid_algo.c +13 ~~-18~~

index a849b74..bcd27c1 100644
@@ -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);

Signature · 1 line

1.5.6.1

Vegard Nossum

author

2008-07-07 20:32 UTC #2 lore raw

Re: [PATCH] rc80211_pid: Fix fast_start parameter handling

Hi,

Quoted 10 lines · 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 <[email protected]>
---
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

Signature · 4 lines

"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

Mattias Nissler

2008-07-07 20:39 UTC #3 lore raw

Re: [PATCH] rc80211_pid: Fix fast_start parameter handling

Quoted 9 lines · Vegard Nossum

Hi,
Quoted 6 lines · 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.

Quoted 8 lines

Quoted 4 lines
Signed-off-by Mattias Nissler <[email protected]>
---
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.

Quoted 5 lines

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

Mattias Nissler

2008-07-07 21:08 UTC #4 lore raw

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

In reply to Vegard Nossum

Signed-off-by Mattias Nissler <[email protected]>

---

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(-)

net/mac80211/rc80211_pid.h +0 -5

index 04afc13..4ea7b97 100644
@@ -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;

net/mac80211/rc80211_pid_algo.c +13 ~~-18~~

index a849b74..bcd27c1 100644
@@ -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);

Signature · 1 line

1.5.6.1

Vegard Nossum

author

2008-07-10 09:58 UTC #5 lore raw

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

Quoted 7 lines · 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.

Quoted 6 lines

Signed-off-by Mattias Nissler <[email protected]>
---
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

Signature · 4 lines

"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

Ingo Molnar

2008-07-10 10:40 UTC #6 lore raw

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

In reply to Mattias Nissler

Quoted 3 lines · Mattias Nissler

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

cool :-)

Ingo