ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Stefan Richter]

Commit bfff68738f1c "ext4: add support for lazy inode table
initialization" added the following build warning:

fs/ext4/super.c: In function 'ext4_lazyinit_thread':
fs/ext4/super.c:2702: warning: 'ret' may be used uninitialized in this function

This warning is due to an actual bug.  But I don't know what the fix is.
-- 
Stefan Richter
-=====-==-=- =-=- =====
http://arcgraph.de/sr/

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Lukas Czerner]

On Sun, 31 Oct 2010, Stefan Richter wrote:

> Commit bfff68738f1c "ext4: add support for lazy inode table
> initialization" added the following build warning:
> 
> fs/ext4/super.c: In function 'ext4_lazyinit_thread':
> fs/ext4/super.c:2702: warning: 'ret' may be used uninitialized in this function
> 
> This warning is due to an actual bug.  But I don't know what the fix is.
> 

Hi Stefan,

thank you for noticing this, because I actually do not see the warning
(I wonder why...), but it is definitely a bug, so the trivial patch below
should fix that.

Thanks!
-Lukas

>From 3594c17e3f724356ea8cc0a1579ab09990a771ac Mon Sep 17 00:00:00 2001
From: Lukas Czerner <lczerner@redhat.com>
Date: Mon, 1 Nov 2010 15:44:54 +0100
Subject: [PATCH] ext4: fix using uninitialized variable

The ret variable might be used uninitialized. Fix this by  initializing
it in definition.

Signed-off-by: "Lukas Czerner" <lczerner@redhat.com>
Reported-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
---
 fs/ext4/super.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8d1d942..aff17cf 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2699,7 +2699,7 @@ static int ext4_lazyinit_thread(void *arg)
 	struct ext4_li_request *elr;
 	unsigned long next_wakeup;
 	DEFINE_WAIT(wait);
-	int ret;
+	int ret = 0;
 
 	BUG_ON(NULL == eli);
 
-- 
1.7.2.3

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Stefan Richter]

Lukas Czerner wrote:
> thank you for noticing this, because I actually do not see the warning
> (I wonder why...),

Probably because of different gcc versions.  Here: 4.3.4
-- 
Stefan Richter
-=====-==-=- =-== ----=
http://arcgraph.de/sr/

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Ted Ts'o]

On Mon, Nov 01, 2010 at 04:27:26PM +0100, Lukas Czerner wrote:
> 
> thank you for noticing this, because I actually do not see the warning
> (I wonder why...), but it is definitely a bug, so the trivial patch below
> should fix that.

This is a slightly less trivial fix that eliminates the need for the
"ret" variable entirely.

						- Ted

commit e048924538f0c62d18306e2fea0e22dac0140f6e
Author: Theodore Ts'o <tytso@mit.edu>
Date:   Tue Nov 2 14:19:30 2010 -0400

    ext4: "ret" may be used uninitialized in ext4_lazyinit_thread()
    
    Newer GCC's reported the following build warning:
    
       fs/ext4/super.c: In function 'ext4_lazyinit_thread':
       fs/ext4/super.c:2702: warning: 'ret' may be used uninitialized in this function
    
    Fix it by removing the need for the ret variable in the first place.
    
    Signed-off-by: "Lukas Czerner" <lczerner@redhat.com>
    Reported-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8d1d942..4d7ef31 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2699,7 +2699,6 @@ static int ext4_lazyinit_thread(void *arg)
 	struct ext4_li_request *elr;
 	unsigned long next_wakeup;
 	DEFINE_WAIT(wait);
-	int ret;
 
 	BUG_ON(NULL == eli);
 
@@ -2723,13 +2722,12 @@ cont_thread:
 			elr = list_entry(pos, struct ext4_li_request,
 					 lr_request);
 
-			if (time_after_eq(jiffies, elr->lr_next_sched))
-				ret = ext4_run_li_request(elr);

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[kevin granade]

On Tue, Nov 2, 2010 at 1:29 PM, Ted Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Nov 01, 2010 at 04:27:26PM +0100, Lukas Czerner wrote:
> >
> > thank you for noticing this, because I actually do not see the warning
> > (I wonder why...), but it is definitely a bug, so the trivial patch below
> > should fix that.
>
> This is a slightly less trivial fix that eliminates the need for the
> "ret" variable entirely.
>
>                                                - Ted
>
> commit e048924538f0c62d18306e2fea0e22dac0140f6e
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Tue Nov 2 14:19:30 2010 -0400
>
>    ext4: "ret" may be used uninitialized in ext4_lazyinit_thread()
>
>    Newer GCC's reported the following build warning:
>
>       fs/ext4/super.c: In function 'ext4_lazyinit_thread':
>       fs/ext4/super.c:2702: warning: 'ret' may be used uninitialized in this function
>
>    Fix it by removing the need for the ret variable in the first place.
>
>    Signed-off-by: "Lukas Czerner" <lczerner@redhat.com>
>    Reported-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
>    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 8d1d942..4d7ef31 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2699,7 +2699,6 @@ static int ext4_lazyinit_thread(void *arg)
>        struct ext4_li_request *elr;
>        unsigned long next_wakeup;
>        DEFINE_WAIT(wait);
> -       int ret;
>
>        BUG_ON(NULL == eli);
>
> @@ -2723,13 +2722,12 @@ cont_thread:
>                        elr = list_entry(pos, struct ext4_li_request,
>                                         lr_request);
>
> -                       if (time_after_eq(jiffies, elr->lr_next_sched))
> -                               ret = ext4_run_li_request(elr);
> -
> -                       if (ret) {
> -                               ret = 0;
> -                               ext4_remove_li_request(elr);
> -                               continue;
> +                       if (time_after_eq(jiffies, elr->lr_next_sched)) {
> +                               if (ext4_run_li_request(elr) != 0) {
> +                                       /* error, remove the lazy_init job */
> +                                       ext4_remove_li_request(elr);
> +                                       continue;
> +                               }
>                        }
>
>                        if (time_before(elr->lr_next_sched, next_wakeup))

What do you think about this option for the second hunk? (not anything-tested)

@@ -2723,13 +2722,11 @@ cont_thread:
                       elr = list_entry(pos, struct ext4_li_request,
                                        lr_request);
-                       if (time_after_eq(jiffies, elr->lr_next_sched))
-                               ret = ext4_run_li_request(elr);
-
-                       if (ret) {
-                               ret = 0;
-                               ext4_remove_li_request(elr);
-                               continue;
+                       if (time_after_eq(jiffies, elr->lr_next_sched) &&
+                           ext4_run_li_request(elr) != 0) {
+                               /* error, remove the lazy_init job */
+                               ext4_remove_li_request(elr);
+                               continue;
                       }

                       if (time_before(elr->lr_next_sched, next_wakeup))
--

Though obviously it's a pretty subjective style issue.
Kevin Granade

> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Lukas Czerner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4232 bytes --]

On Tue, 2 Nov 2010, kevin granade wrote:

> On Tue, Nov 2, 2010 at 1:29 PM, Ted Ts'o <tytso@mit.edu> wrote:
> >
> > On Mon, Nov 01, 2010 at 04:27:26PM +0100, Lukas Czerner wrote:
> > >
> > > thank you for noticing this, because I actually do not see the warning
> > > (I wonder why...), but it is definitely a bug, so the trivial patch below
> > > should fix that.
> >
> > This is a slightly less trivial fix that eliminates the need for the
> > "ret" variable entirely.
> >
> >                                                - Ted
> >
> > commit e048924538f0c62d18306e2fea0e22dac0140f6e
> > Author: Theodore Ts'o <tytso@mit.edu>
> > Date:   Tue Nov 2 14:19:30 2010 -0400
> >
> >    ext4: "ret" may be used uninitialized in ext4_lazyinit_thread()
> >
> >    Newer GCC's reported the following build warning:
> >
> >       fs/ext4/super.c: In function 'ext4_lazyinit_thread':
> >       fs/ext4/super.c:2702: warning: 'ret' may be used uninitialized in this function
> >
> >    Fix it by removing the need for the ret variable in the first place.
> >
> >    Signed-off-by: "Lukas Czerner" <lczerner@redhat.com>
> >    Reported-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
> >    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> >
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 8d1d942..4d7ef31 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2699,7 +2699,6 @@ static int ext4_lazyinit_thread(void *arg)
> >        struct ext4_li_request *elr;
> >        unsigned long next_wakeup;
> >        DEFINE_WAIT(wait);
> > -       int ret;
> >
> >        BUG_ON(NULL == eli);
> >
> > @@ -2723,13 +2722,12 @@ cont_thread:
> >                        elr = list_entry(pos, struct ext4_li_request,
> >                                         lr_request);
> >
> > -                       if (time_after_eq(jiffies, elr->lr_next_sched))
> > -                               ret = ext4_run_li_request(elr);
> > -
> > -                       if (ret) {
> > -                               ret = 0;
> > -                               ext4_remove_li_request(elr);
> > -                               continue;
> > +                       if (time_after_eq(jiffies, elr->lr_next_sched)) {
> > +                               if (ext4_run_li_request(elr) != 0) {
> > +                                       /* error, remove the lazy_init job */
> > +                                       ext4_remove_li_request(elr);
> > +                                       continue;
> > +                               }
> >                        }
> >
> >                        if (time_before(elr->lr_next_sched, next_wakeup))
> 
> What do you think about this option for the second hunk? (not anything-tested)
> 
> @@ -2723,13 +2722,11 @@ cont_thread:
>                        elr = list_entry(pos, struct ext4_li_request,
>                                         lr_request);
> -                       if (time_after_eq(jiffies, elr->lr_next_sched))
> -                               ret = ext4_run_li_request(elr);
> -
> -                       if (ret) {
> -                               ret = 0;
> -                               ext4_remove_li_request(elr);
> -                               continue;
> +                       if (time_after_eq(jiffies, elr->lr_next_sched) &&
> +                           ext4_run_li_request(elr) != 0) {
> +                               /* error, remove the lazy_init job */
> +                               ext4_remove_li_request(elr);
> +                               continue;
>                        }
> 
>                        if (time_before(elr->lr_next_sched, next_wakeup))
> --
> 
> Though obviously it's a pretty subjective style issue.
> Kevin Granade

Hmm this relies on the fact that if the first part of the condition
would not be true, the second part (after and) would never be invoked,
however I am not really sure that we can rely on that on every
architecture, or can we ?

> 
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

Thanks!

-Lukas

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Nick Bowler]

On 2010-11-02 15:19 -0400, Lukas Czerner wrote:
> Hmm this relies on the fact that if the first part of the condition
> would not be true, the second part (after and) would never be invoked,
> however I am not really sure that we can rely on that on every
> architecture, or can we ?

This behaviour of the && operator is guaranteed by the C standard.

-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Stefan Richter]

Lukas Czerner wrote:
> On Tue, 2 Nov 2010, kevin granade wrote:
>> +                       if (time_after_eq(jiffies, elr->lr_next_sched) &&
>> +                           ext4_run_li_request(elr) != 0) {
>> +                               /* error, remove the lazy_init job */
>> +                               ext4_remove_li_request(elr);
>> +                               continue;
>>                        }
>>
>>                        if (time_before(elr->lr_next_sched, next_wakeup))
>> --
>>
>> Though obviously it's a pretty subjective style issue.
>> Kevin Granade
> 
> Hmm this relies on the fact that if the first part of the condition
> would not be true, the second part (after and) would never be invoked,
> however I am not really sure that we can rely on that on every
> architecture, or can we ?

It is not about architecture but a C language feature.  It is relied upon
everywhere in the kernel.  For example,

	if (p != NULL && p->m == something) {
		...

is very common.  The || operator has the same property:  Evaluation stops as
soon as the end result is known.  I do not know since when this minimum
evaluation feature is guaranteed in the language, but it has to be ages now.

(This is not an endorsement of one or the other coding of the patch. :-)
-- 
Stefan Richter
-=====-==-=- =-== ---=-
http://arcgraph.de/sr/

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Brian Gitonga Marete]

Indeed. In clause 4 of Section 6.5.13 of C99, short circuit evaluation
is guaranteed in addition to left to right evaluation.

On 11/2/10, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Lukas Czerner wrote:
>> On Tue, 2 Nov 2010, kevin granade wrote:
>>> +                       if (time_after_eq(jiffies, elr->lr_next_sched) &&
>>> +                           ext4_run_li_request(elr) != 0) {
>>> +                               /* error, remove the lazy_init job */
>>> +                               ext4_remove_li_request(elr);
>>> +                               continue;
>>>                        }
>>>
>>>                        if (time_before(elr->lr_next_sched, next_wakeup))
>>> --
>>>
>>> Though obviously it's a pretty subjective style issue.
>>> Kevin Granade
>>
>> Hmm this relies on the fact that if the first part of the condition
>> would not be true, the second part (after and) would never be invoked,
>> however I am not really sure that we can rely on that on every
>> architecture, or can we ?
>
> It is not about architecture but a C language feature.  It is relied upon
> everywhere in the kernel.  For example,
>
> 	if (p != NULL && p->m == something) {
> 		...
>
> is very common.  The || operator has the same property:  Evaluation stops as
> soon as the end result is known.  I do not know since when this minimum
> evaluation feature is guaranteed in the language, but it has to be ages now.
>
> (This is not an endorsement of one or the other coding of the patch. :-)
> --
> Stefan Richter
> -=====-==-=- =-== ---=-
> http://arcgraph.de/sr/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Brian Gitonga Marete
Toshnix Systems
Tel: +254722151590

Re: ext4_lazyinit_thread: 'ret' may be used uninitialized in this function

[Lukas Czerner]

On Tue, 2 Nov 2010, Ted Ts'o wrote:

> On Mon, Nov 01, 2010 at 04:27:26PM +0100, Lukas Czerner wrote:
> > 
> > thank you for noticing this, because I actually do not see the warning
> > (I wonder why...), but it is definitely a bug, so the trivial patch below
> > should fix that.
> 
> This is a slightly less trivial fix that eliminates the need for the
> "ret" variable entirely.
> 
> 						- Ted
> 
> commit e048924538f0c62d18306e2fea0e22dac0140f6e
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Tue Nov 2 14:19:30 2010 -0400
> 
>     ext4: "ret" may be used uninitialized in ext4_lazyinit_thread()
>     
>     Newer GCC's reported the following build warning:
>     
>        fs/ext4/super.c: In function 'ext4_lazyinit_thread':
>        fs/ext4/super.c:2702: warning: 'ret' may be used uninitialized in this function
>     
>     Fix it by removing the need for the ret variable in the first place.
>     
>     Signed-off-by: "Lukas Czerner" <lczerner@redhat.com>
>     Reported-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
>     Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 8d1d942..4d7ef31 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2699,7 +2699,6 @@ static int ext4_lazyinit_thread(void *arg)
>  	struct ext4_li_request *elr;
>  	unsigned long next_wakeup;
>  	DEFINE_WAIT(wait);
> -	int ret;
>  
>  	BUG_ON(NULL == eli);
>  
> @@ -2723,13 +2722,12 @@ cont_thread:
>  			elr = list_entry(pos, struct ext4_li_request,
>  					 lr_request);
>  
> -			if (time_after_eq(jiffies, elr->lr_next_sched))
> -				ret = ext4_run_li_request(elr);
> -
> -			if (ret) {
> -				ret = 0;
> -				ext4_remove_li_request(elr);
> -				continue;
> +			if (time_after_eq(jiffies, elr->lr_next_sched)) {
> +				if (ext4_run_li_request(elr) != 0) {
> +					/* error, remove the lazy_init job */

It is not removed only in the case of error, but even if it hits the
last group, so I would just omit the "error" part.

> +					ext4_remove_li_request(elr);
> +					continue;
> +				}
>  			}
>  
>  			if (time_before(elr->lr_next_sched, next_wakeup))
> 

Otherwise looks good to me.

-Lukas