Re: Possible usage of uninitalized task_ratelimit variable in mm/page-writeback.c

Re: Possible usage of uninitalized task_ratelimit variable in mm/page-writeback.c

[Wu Fengguang]

On Mon, Nov 07, 2011 at 04:18:24PM +0800, Witold Baryluk wrote:
> Hi,
> 
> I found a minor issue when compiling kernel today
> 
> 
>   CC      mm/page-writeback.o
> mm/page-writeback.c: In function ‘balance_dirty_pages_ratelimited_nr’:
> include/trace/events/writeback.h:281:1: warning: ‘task_ratelimit’ may be used uninitialized in this function [-Wuninitialized]
> mm/page-writeback.c:1018:16: note: ‘task_ratelimit’ was declared here
> 
> Indeed in balance_dirty_pages a task_ratelimit may be not initialized
> (initialization skiped by goto pause;), and then used when calling
> tracing hook.

Witold, thanks for the report! This patch should fix the bug.

Thanks,
Fengguang
---

writeback: fix uninitialized task_ratelimit

In balance_dirty_pages() task_ratelimit may be not initialized
(initialization skiped by goto pause;), and then used when calling
tracing hook.

Fix it by moving the task_ratelimit assignment before goto pause.

Reported-by: Witold Baryluk <baryluk@smp.if.uj.edu.pl>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 mm/page-writeback.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux.orig/mm/page-writeback.c	2011-11-07 17:07:04.080000043 +0800
+++ linux/mm/page-writeback.c	2011-11-07 17:08:43.232000031 +0800
@@ -1097,13 +1097,13 @@ static void balance_dirty_pages(struct a
 		pos_ratio = bdi_position_ratio(bdi, dirty_thresh,
 					       background_thresh, nr_dirty,
 					       bdi_thresh, bdi_dirty);
-		if (unlikely(pos_ratio == 0)) {
+		task_ratelimit = (u64)dirty_ratelimit *
+					pos_ratio >> RATELIMIT_CALC_SHIFT;
+		if (unlikely(task_ratelimit == 0)) {
 			pause = max_pause;
 			goto pause;
 		}
-		task_ratelimit = (u64)dirty_ratelimit *
-					pos_ratio >> RATELIMIT_CALC_SHIFT;
-		pause = (HZ * pages_dirtied) / (task_ratelimit | 1);
+		pause = (HZ * pages_dirtied) / task_ratelimit;
 		if (unlikely(pause <= 0)) {
 			trace_balance_dirty_pages(bdi,
 						  dirty_thresh,
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Possible usage of uninitalized task_ratelimit variable in mm/page-writeback.c

[Witold Baryluk]

On 11-07 17:17, Wu Fengguang wrote:
> On Mon, Nov 07, 2011 at 04:18:24PM +0800, Witold Baryluk wrote:
> > Hi,
> > 
> > I found a minor issue when compiling kernel today
> > 
> > 
> >   CC      mm/page-writeback.o
> > mm/page-writeback.c: In function ‘balance_dirty_pages_ratelimited_nr’:
> > include/trace/events/writeback.h:281:1: warning: ‘task_ratelimit’ may be used uninitialized in this function [-Wuninitialized]
> > mm/page-writeback.c:1018:16: note: ‘task_ratelimit’ was declared here
> > 
> > Indeed in balance_dirty_pages a task_ratelimit may be not initialized
> > (initialization skiped by goto pause;), and then used when calling
> > tracing hook.
> 
> Witold, thanks for the report! This patch should fix the bug.
> 
> Thanks,
> Fengguang
> ---
> 
> writeback: fix uninitialized task_ratelimit
> 
> In balance_dirty_pages() task_ratelimit may be not initialized
> (initialization skiped by goto pause;), and then used when calling
> tracing hook.
> 
> Fix it by moving the task_ratelimit assignment before goto pause.
> 
> Reported-by: Witold Baryluk <baryluk@smp.if.uj.edu.pl>
> Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> ---
>  mm/page-writeback.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux.orig/mm/page-writeback.c	2011-11-07 17:07:04.080000043 +0800
> +++ linux/mm/page-writeback.c	2011-11-07 17:08:43.232000031 +0800
> @@ -1097,13 +1097,13 @@ static void balance_dirty_pages(struct a
>  		pos_ratio = bdi_position_ratio(bdi, dirty_thresh,
>  					       background_thresh, nr_dirty,
>  					       bdi_thresh, bdi_dirty);
> -		if (unlikely(pos_ratio == 0)) {
> +		task_ratelimit = (u64)dirty_ratelimit *
> +					pos_ratio >> RATELIMIT_CALC_SHIFT;
> +		if (unlikely(task_ratelimit == 0)) {
>  			pause = max_pause;
>  			goto pause;
>  		}
> -		task_ratelimit = (u64)dirty_ratelimit *
> -					pos_ratio >> RATELIMIT_CALC_SHIFT;
> -		pause = (HZ * pages_dirtied) / (task_ratelimit | 1);
> +		pause = (HZ * pages_dirtied) / task_ratelimit;
>  		if (unlikely(pause <= 0)) {
>  			trace_balance_dirty_pages(bdi,
>  						  dirty_thresh,

Thanks.

This is very nice patch, fixes warning,
and simplifies logic. I have no other objections.

(I do not remember what have bigger priority, * or >>
- i guess * - but additional paranthesis can help. :D )

Regards,
Witek

-- 
Witold Baryluk

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Possible usage of uninitalized task_ratelimit variable in mm/page-writeback.c

[Wu Fengguang]

On Mon, Nov 07, 2011 at 07:05:58PM +0800, Witold Baryluk wrote:
> On 11-07 17:17, Wu Fengguang wrote:
> > On Mon, Nov 07, 2011 at 04:18:24PM +0800, Witold Baryluk wrote:
> > > Hi,
> > > 
> > > I found a minor issue when compiling kernel today
> > > 
> > > 
> > >   CC      mm/page-writeback.o
> > > mm/page-writeback.c: In function ‘balance_dirty_pages_ratelimited_nr’:
> > > include/trace/events/writeback.h:281:1: warning: ‘task_ratelimit’ may be used uninitialized in this function [-Wuninitialized]
> > > mm/page-writeback.c:1018:16: note: ‘task_ratelimit’ was declared here
> > > 
> > > Indeed in balance_dirty_pages a task_ratelimit may be not initialized
> > > (initialization skiped by goto pause;), and then used when calling
> > > tracing hook.
> > 
> > Witold, thanks for the report! This patch should fix the bug.
> > 
> > Thanks,
> > Fengguang
> > ---
> > 
> > writeback: fix uninitialized task_ratelimit
> > 
> > In balance_dirty_pages() task_ratelimit may be not initialized
> > (initialization skiped by goto pause;), and then used when calling
> > tracing hook.
> > 
> > Fix it by moving the task_ratelimit assignment before goto pause.
> > 
> > Reported-by: Witold Baryluk <baryluk@smp.if.uj.edu.pl>
> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
> > ---
> >  mm/page-writeback.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > --- linux.orig/mm/page-writeback.c	2011-11-07 17:07:04.080000043 +0800
> > +++ linux/mm/page-writeback.c	2011-11-07 17:08:43.232000031 +0800
> > @@ -1097,13 +1097,13 @@ static void balance_dirty_pages(struct a
> >  		pos_ratio = bdi_position_ratio(bdi, dirty_thresh,
> >  					       background_thresh, nr_dirty,
> >  					       bdi_thresh, bdi_dirty);
> > -		if (unlikely(pos_ratio == 0)) {
> > +		task_ratelimit = (u64)dirty_ratelimit *
> > +					pos_ratio >> RATELIMIT_CALC_SHIFT;
> > +		if (unlikely(task_ratelimit == 0)) {
> >  			pause = max_pause;
> >  			goto pause;
> >  		}
> > -		task_ratelimit = (u64)dirty_ratelimit *
> > -					pos_ratio >> RATELIMIT_CALC_SHIFT;
> > -		pause = (HZ * pages_dirtied) / (task_ratelimit | 1);
> > +		pause = (HZ * pages_dirtied) / task_ratelimit;
> >  		if (unlikely(pause <= 0)) {
> >  			trace_balance_dirty_pages(bdi,
> >  						  dirty_thresh,
> 
> Thanks.
> 
> This is very nice patch, fixes warning,
> and simplifies logic. I have no other objections.
> 
> (I do not remember what have bigger priority, * or >>
> - i guess * - but additional paranthesis can help. :D )

Good point! Just added/removed parentheses according to your suggestions :)

Thanks,
Fengguang
---
writeback: fix uninitialized task_ratelimit

In balance_dirty_pages() task_ratelimit may be not initialized
(initialization skiped by goto pause;), and then used when calling
tracing hook.

Fix it by moving the task_ratelimit assignment before goto pause.

Reported-by: Witold Baryluk <baryluk@smp.if.uj.edu.pl>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
 mm/page-writeback.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux.orig/mm/page-writeback.c	2011-11-07 19:11:39.660000042 +0800
+++ linux/mm/page-writeback.c	2011-11-07 19:12:19.856000041 +0800
@@ -1095,13 +1095,13 @@ static void balance_dirty_pages(struct a
 		pos_ratio = bdi_position_ratio(bdi, dirty_thresh,
 					       background_thresh, nr_dirty,
 					       bdi_thresh, bdi_dirty);
-		if (unlikely(pos_ratio == 0)) {
+		task_ratelimit = ((u64)dirty_ratelimit * pos_ratio) >>
+							RATELIMIT_CALC_SHIFT;
+		if (unlikely(task_ratelimit == 0)) {
 			pause = max_pause;
 			goto pause;
 		}
-		task_ratelimit = (u64)dirty_ratelimit *
-					pos_ratio >> RATELIMIT_CALC_SHIFT;
-		pause = (HZ * pages_dirtied) / (task_ratelimit | 1);
+		pause = HZ * pages_dirtied / task_ratelimit;
 		if (unlikely(pause <= 0)) {
 			trace_balance_dirty_pages(bdi,
 						  dirty_thresh,
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html