[BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

gcc points out some obviously broken code in linux-next

mm/vmalloc.c: In function 'pcpu_get_vm_areas':
mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    insert_vmap_area_augment(lva, &va->rb_node,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     &free_vmap_area_root, &free_vmap_area_list);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mm/vmalloc.c:916:20: note: 'lva' was declared here
  struct vmap_area *lva;
                    ^~~

Remove the obviously broken code. This is almost certainly
not the correct solution, but it's what I have applied locally
to get a clean build again.

Please fix this properly.

Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 mm/vmalloc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index a9213fc3802d..bfcf0124a773 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -984,14 +984,9 @@ adjust_va_to_fit_type(struct vmap_area *va,
 		return -1;
 	}
 
-	if (type != FL_FIT_TYPE) {
+	if (type == FL_FIT_TYPE)
 		augment_tree_propagate_from(va);
 
-		if (type == NE_FIT_TYPE)
-			insert_vmap_area_augment(lva, &va->rb_node,
-				&free_vmap_area_root, &free_vmap_area_list);
-	}
-
 	return 0;
 }
 
-- 
2.20.0

[BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

gcc points out some obviously broken code in linux-next

mm/vmalloc.c: In function 'pcpu_get_vm_areas':
mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    insert_vmap_area_augment(lva, &va->rb_node,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     &free_vmap_area_root, &free_vmap_area_list);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mm/vmalloc.c:916:20: note: 'lva' was declared here
  struct vmap_area *lva;
                    ^~~

Remove the obviously broken code. This is almost certainly
not the correct solution, but it's what I have applied locally
to get a clean build again.

Please fix this properly.

Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 mm/vmalloc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index a9213fc3802d..bfcf0124a773 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -984,14 +984,9 @@ adjust_va_to_fit_type(struct vmap_area *va,
 		return -1;
 	}
 
-	if (type != FL_FIT_TYPE) {
+	if (type == FL_FIT_TYPE)
 		augment_tree_propagate_from(va);
 
-		if (type == NE_FIT_TYPE)
-			insert_vmap_area_augment(lva, &va->rb_node,
-				&free_vmap_area_root, &free_vmap_area_list);
-	}
-
 	return 0;
 }
 
-- 
2.20.0

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Roman Penyaev]

On 2019-06-17 14:14, Arnd Bergmann wrote:
> gcc points out some obviously broken code in linux-next
> 
> mm/vmalloc.c: In function 'pcpu_get_vm_areas':
> mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>     insert_vmap_area_augment(lva, &va->rb_node,
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      &free_vmap_area_root, &free_vmap_area_list);
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> mm/vmalloc.c:916:20: note: 'lva' was declared here
>   struct vmap_area *lva;
>                     ^~~
> 
> Remove the obviously broken code. This is almost certainly
> not the correct solution, but it's what I have applied locally
> to get a clean build again.
> 
> Please fix this properly.
> 
> Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap
> allocation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  mm/vmalloc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index a9213fc3802d..bfcf0124a773 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -984,14 +984,9 @@ adjust_va_to_fit_type(struct vmap_area *va,
>  		return -1;
>  	}
> 
> -	if (type != FL_FIT_TYPE) {
> +	if (type == FL_FIT_TYPE)
>  		augment_tree_propagate_from(va);
> 
> -		if (type == NE_FIT_TYPE)
> -			insert_vmap_area_augment(lva, &va->rb_node,
> -				&free_vmap_area_root, &free_vmap_area_list);
> -	}
> -
>  	return 0;
>  }


Hi Arnd,

Seems the proper fix is just setting lva to NULL.  The only place
where lva is allocated and then used is when type == NE_FIT_TYPE,
so according to my shallow understanding of the code everything
should be fine.

--
Roman

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 3:49 PM Roman Penyaev <rpenyaev@suse.de> wrote:
> >               augment_tree_propagate_from(va);
> >
> > -             if (type == NE_FIT_TYPE)
> > -                     insert_vmap_area_augment(lva, &va->rb_node,
> > -                             &free_vmap_area_root, &free_vmap_area_list);
> > -     }
> > -
> >       return 0;
> >  }
>
>
> Hi Arnd,
>
> Seems the proper fix is just setting lva to NULL.  The only place
> where lva is allocated and then used is when type == NE_FIT_TYPE,
> so according to my shallow understanding of the code everything
> should be fine.

I don't see how NULL could work here. insert_vmap_area_augment()
passes the va pointer into find_va_links() and link_va(), both of
which dereference the pointer, see

static void
insert_vmap_area_augment(struct vmap_area *va,
        struct rb_node *from, struct rb_root *root,
        struct list_head *head)
{
        struct rb_node **link;
        struct rb_node *parent;

        if (from)
                link = find_va_links(va, NULL, from, &parent);
        else
                link = find_va_links(va, root, NULL, &parent);

        link_va(va, root, parent, link, head);
        augment_tree_propagate_from(va);
}

static __always_inline struct rb_node **
find_va_links(struct vmap_area *va,
        struct rb_root *root, struct rb_node *from,
        struct rb_node **parent)
{
       ...
                       if (va->va_start < tmp_va->va_end &&
                                va->va_end <= tmp_va->va_start)
       ...
}

static __always_inline void
link_va(struct vmap_area *va, struct rb_root *root,
        struct rb_node *parent, struct rb_node **link, struct list_head *head)
{
        ...
        rb_link_node(&va->rb_node, parent, link);
        ...
}

       Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 3:49 PM Roman Penyaev <rpenyaev@suse.de> wrote:
> >               augment_tree_propagate_from(va);
> >
> > -             if (type == NE_FIT_TYPE)
> > -                     insert_vmap_area_augment(lva, &va->rb_node,
> > -                             &free_vmap_area_root, &free_vmap_area_list);
> > -     }
> > -
> >       return 0;
> >  }
>
>
> Hi Arnd,
>
> Seems the proper fix is just setting lva to NULL.  The only place
> where lva is allocated and then used is when type == NE_FIT_TYPE,
> so according to my shallow understanding of the code everything
> should be fine.

I don't see how NULL could work here. insert_vmap_area_augment()
passes the va pointer into find_va_links() and link_va(), both of
which dereference the pointer, see

static void
insert_vmap_area_augment(struct vmap_area *va,
        struct rb_node *from, struct rb_root *root,
        struct list_head *head)
{
        struct rb_node **link;
        struct rb_node *parent;

        if (from)
                link = find_va_links(va, NULL, from, &parent);
        else
                link = find_va_links(va, root, NULL, &parent);

        link_va(va, root, parent, link, head);
        augment_tree_propagate_from(va);
}

static __always_inline struct rb_node **
find_va_links(struct vmap_area *va,
        struct rb_root *root, struct rb_node *from,
        struct rb_node **parent)
{
       ...
                       if (va->va_start < tmp_va->va_end &&
                                va->va_end <= tmp_va->va_start)
       ...
}

static __always_inline void
link_va(struct vmap_area *va, struct rb_root *root,
        struct rb_node *parent, struct rb_node **link, struct list_head *head)
{
        ...
        rb_link_node(&va->rb_node, parent, link);
        ...
}

       Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Roman Penyaev]

On 2019-06-17 16:04, Arnd Bergmann wrote:
> On Mon, Jun 17, 2019 at 3:49 PM Roman Penyaev <rpenyaev@suse.de> wrote:
>> >               augment_tree_propagate_from(va);
>> >
>> > -             if (type == NE_FIT_TYPE)
>> > -                     insert_vmap_area_augment(lva, &va->rb_node,
>> > -                             &free_vmap_area_root, &free_vmap_area_list);
>> > -     }
>> > -
>> >       return 0;
>> >  }
>> 
>> 
>> Hi Arnd,
>> 
>> Seems the proper fix is just setting lva to NULL.  The only place
>> where lva is allocated and then used is when type == NE_FIT_TYPE,
>> so according to my shallow understanding of the code everything
>> should be fine.
> 
> I don't see how NULL could work here. insert_vmap_area_augment()
> passes the va pointer into find_va_links() and link_va(), both of
> which dereference the pointer, see

Exactly, but insert_vmap_area_augement() accepts 'va', not 'lva',
but in your variant 'va' is already freed (see type == FL_FIT_TYPE
branch, on top of that function).  So that should be use-after-free.

--
Roman

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Uladzislau Rezki]

On Mon, Jun 17, 2019 at 02:14:11PM +0200, Arnd Bergmann wrote:
> gcc points out some obviously broken code in linux-next
> 
> mm/vmalloc.c: In function 'pcpu_get_vm_areas':
> mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     insert_vmap_area_augment(lva, &va->rb_node,
>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      &free_vmap_area_root, &free_vmap_area_list);
>      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> mm/vmalloc.c:916:20: note: 'lva' was declared here
>   struct vmap_area *lva;
>                     ^~~
> 
> Remove the obviously broken code. This is almost certainly
> not the correct solution, but it's what I have applied locally
> to get a clean build again.
> 
> Please fix this properly.
> 
> Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  mm/vmalloc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index a9213fc3802d..bfcf0124a773 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -984,14 +984,9 @@ adjust_va_to_fit_type(struct vmap_area *va,
>  		return -1;
>  	}
>  
> -	if (type != FL_FIT_TYPE) {
> +	if (type == FL_FIT_TYPE)
>  		augment_tree_propagate_from(va);
>  
> -		if (type == NE_FIT_TYPE)
> -			insert_vmap_area_augment(lva, &va->rb_node,
> -				&free_vmap_area_root, &free_vmap_area_list);
> -	}
> -
>  	return 0;
>  }
>  
> -- 
> 2.20.0
> 
Please do not apply this. It will just break everything. As Roman
pointed we can just set lva = NULL; in the beginning to make GCC happy. 
For some reason GCC decides that it can be used uninitialized, but that
is not true.

--
Vlad Rezki

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 4:12 PM Uladzislau Rezki <urezki@gmail.com> wrote:
>
> On Mon, Jun 17, 2019 at 02:14:11PM +0200, Arnd Bergmann wrote:
> > gcc points out some obviously broken code in linux-next
> >
> > mm/vmalloc.c: In function 'pcpu_get_vm_areas':
> > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >     insert_vmap_area_augment(lva, &va->rb_node,
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >      &free_vmap_area_root, &free_vmap_area_list);
> >      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > mm/vmalloc.c:916:20: note: 'lva' was declared here
> >   struct vmap_area *lva;
> >                     ^~~
> >
> > Remove the obviously broken code. This is almost certainly
> > not the correct solution, but it's what I have applied locally
> > to get a clean build again.
> >
> > Please fix this properly.
> >

> >
> Please do not apply this. It will just break everything.

As I wrote in my description, this was purely meant as a bug
report, not a patch to be applied.

> As Roman pointed we can just set lva = NULL; in the beginning to make GCC happy.
> For some reason GCC decides that it can be used uninitialized, but that
> is not true.

I got confused by the similarly named FL_FIT_TYPE/NE_FIT_TYPE
constants and misread this as only getting run in the case where it is
not initialized, but you are right that it always is initialized here.

I see now that the actual cause of the warning is the 'while' loop in
augment_tree_propagate_from(). gcc is unable to keep track of
the state of the 'lva' variable beyond that and prints a bogus warning.

        Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 4:12 PM Uladzislau Rezki <urezki@gmail.com> wrote:
>
> On Mon, Jun 17, 2019 at 02:14:11PM +0200, Arnd Bergmann wrote:
> > gcc points out some obviously broken code in linux-next
> >
> > mm/vmalloc.c: In function 'pcpu_get_vm_areas':
> > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >     insert_vmap_area_augment(lva, &va->rb_node,
> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >      &free_vmap_area_root, &free_vmap_area_list);
> >      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > mm/vmalloc.c:916:20: note: 'lva' was declared here
> >   struct vmap_area *lva;
> >                     ^~~
> >
> > Remove the obviously broken code. This is almost certainly
> > not the correct solution, but it's what I have applied locally
> > to get a clean build again.
> >
> > Please fix this properly.
> >

> >
> Please do not apply this. It will just break everything.

As I wrote in my description, this was purely meant as a bug
report, not a patch to be applied.

> As Roman pointed we can just set lva = NULL; in the beginning to make GCC happy.
> For some reason GCC decides that it can be used uninitialized, but that
> is not true.

I got confused by the similarly named FL_FIT_TYPE/NE_FIT_TYPE
constants and misread this as only getting run in the case where it is
not initialized, but you are right that it always is initialized here.

I see now that the actual cause of the warning is the 'while' loop in
augment_tree_propagate_from(). gcc is unable to keep track of
the state of the 'lva' variable beyond that and prints a bogus warning.

        Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Roman Penyaev]

On 2019-06-17 16:44, Arnd Bergmann wrote:
> On Mon, Jun 17, 2019 at 4:12 PM Uladzislau Rezki <urezki@gmail.com> 
> wrote:
>> 
>> On Mon, Jun 17, 2019 at 02:14:11PM +0200, Arnd Bergmann wrote:
>> > gcc points out some obviously broken code in linux-next
>> >
>> > mm/vmalloc.c: In function 'pcpu_get_vm_areas':
>> > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> >     insert_vmap_area_augment(lva, &va->rb_node,
>> >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> >      &free_vmap_area_root, &free_vmap_area_list);
>> >      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > mm/vmalloc.c:916:20: note: 'lva' was declared here
>> >   struct vmap_area *lva;
>> >                     ^~~
>> >
>> > Remove the obviously broken code. This is almost certainly
>> > not the correct solution, but it's what I have applied locally
>> > to get a clean build again.
>> >
>> > Please fix this properly.
>> >
> 
>> >
>> Please do not apply this. It will just break everything.
> 
> As I wrote in my description, this was purely meant as a bug
> report, not a patch to be applied.

That's a perfect way to attract attention! :)

> 
>> As Roman pointed we can just set lva = NULL; in the beginning to make 
>> GCC happy.
>> For some reason GCC decides that it can be used uninitialized, but 
>> that
>> is not true.
> 
> I got confused by the similarly named FL_FIT_TYPE/NE_FIT_TYPE

Names are indeed very confusing, that is true.  Very easy to mix up 
things.

--
Roman

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 4:44 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jun 17, 2019 at 4:12 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> >
> > On Mon, Jun 17, 2019 at 02:14:11PM +0200, Arnd Bergmann wrote:
> > > gcc points out some obviously broken code in linux-next
> > >
> > > mm/vmalloc.c: In function 'pcpu_get_vm_areas':
> > > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >     insert_vmap_area_augment(lva, &va->rb_node,
> > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >      &free_vmap_area_root, &free_vmap_area_list);
> > >      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > mm/vmalloc.c:916:20: note: 'lva' was declared here
> > >   struct vmap_area *lva;
> > >                     ^~~
> > >
> > > Remove the obviously broken code. This is almost certainly
> > > not the correct solution, but it's what I have applied locally
> > > to get a clean build again.
> > >
> > > Please fix this properly.
> > >
>
> > >
> > Please do not apply this. It will just break everything.
>
> As I wrote in my description, this was purely meant as a bug
> report, not a patch to be applied.
>
> > As Roman pointed we can just set lva = NULL; in the beginning to make GCC happy.
> > For some reason GCC decides that it can be used uninitialized, but that
> > is not true.
>
> I got confused by the similarly named FL_FIT_TYPE/NE_FIT_TYPE
> constants and misread this as only getting run in the case where it is
> not initialized, but you are right that it always is initialized here.
>
> I see now that the actual cause of the warning is the 'while' loop in
> augment_tree_propagate_from(). gcc is unable to keep track of
> the state of the 'lva' variable beyond that and prints a bogus warning.

I managed to un-confuse gcc-8 by turning the if/else if/else into
a switch statement. If you all think this is an acceptable solution,
I'll submit that after some more testing to ensure it addresses
all configurations:

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index a9213fc3802d..5b7e50de008b 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -915,7 +915,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
 {
        struct vmap_area *lva;

-       if (type == FL_FIT_TYPE) {
+       switch (type) {
+       case FL_FIT_TYPE:
                /*
                 * No need to split VA, it fully fits.
                 *
@@ -925,7 +926,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 */
                unlink_va(va, &free_vmap_area_root);
                kmem_cache_free(vmap_area_cachep, va);
-       } else if (type == LE_FIT_TYPE) {
+               break;
+       case LE_FIT_TYPE:
                /*
                 * Split left edge of fit VA.
                 *
@@ -934,7 +936,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 * |-------|-------|
                 */
                va->va_start += size;
-       } else if (type == RE_FIT_TYPE) {
+               break;
+       case RE_FIT_TYPE:
                /*
                 * Split right edge of fit VA.
                 *
@@ -943,7 +946,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 * |-------|-------|
                 */
                va->va_end = nva_start_addr;
-       } else if (type == NE_FIT_TYPE) {
+               break;
+       case NE_FIT_TYPE:
                /*
                 * Split no edge of fit VA.
                 *
@@ -980,7 +984,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 * Shrink this VA to remaining size.
                 */
                va->va_start = nva_start_addr + size;
-       } else {
+               break;
+       default:
                return -1;
        }

       Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 4:44 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jun 17, 2019 at 4:12 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> >
> > On Mon, Jun 17, 2019 at 02:14:11PM +0200, Arnd Bergmann wrote:
> > > gcc points out some obviously broken code in linux-next
> > >
> > > mm/vmalloc.c: In function 'pcpu_get_vm_areas':
> > > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > >     insert_vmap_area_augment(lva, &va->rb_node,
> > >     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >      &free_vmap_area_root, &free_vmap_area_list);
> > >      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > mm/vmalloc.c:916:20: note: 'lva' was declared here
> > >   struct vmap_area *lva;
> > >                     ^~~
> > >
> > > Remove the obviously broken code. This is almost certainly
> > > not the correct solution, but it's what I have applied locally
> > > to get a clean build again.
> > >
> > > Please fix this properly.
> > >
>
> > >
> > Please do not apply this. It will just break everything.
>
> As I wrote in my description, this was purely meant as a bug
> report, not a patch to be applied.
>
> > As Roman pointed we can just set lva = NULL; in the beginning to make GCC happy.
> > For some reason GCC decides that it can be used uninitialized, but that
> > is not true.
>
> I got confused by the similarly named FL_FIT_TYPE/NE_FIT_TYPE
> constants and misread this as only getting run in the case where it is
> not initialized, but you are right that it always is initialized here.
>
> I see now that the actual cause of the warning is the 'while' loop in
> augment_tree_propagate_from(). gcc is unable to keep track of
> the state of the 'lva' variable beyond that and prints a bogus warning.

I managed to un-confuse gcc-8 by turning the if/else if/else into
a switch statement. If you all think this is an acceptable solution,
I'll submit that after some more testing to ensure it addresses
all configurations:

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index a9213fc3802d..5b7e50de008b 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -915,7 +915,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
 {
        struct vmap_area *lva;

-       if (type == FL_FIT_TYPE) {
+       switch (type) {
+       case FL_FIT_TYPE:
                /*
                 * No need to split VA, it fully fits.
                 *
@@ -925,7 +926,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 */
                unlink_va(va, &free_vmap_area_root);
                kmem_cache_free(vmap_area_cachep, va);
-       } else if (type == LE_FIT_TYPE) {
+               break;
+       case LE_FIT_TYPE:
                /*
                 * Split left edge of fit VA.
                 *
@@ -934,7 +936,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 * |-------|-------|
                 */
                va->va_start += size;
-       } else if (type == RE_FIT_TYPE) {
+               break;
+       case RE_FIT_TYPE:
                /*
                 * Split right edge of fit VA.
                 *
@@ -943,7 +946,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 * |-------|-------|
                 */
                va->va_end = nva_start_addr;
-       } else if (type == NE_FIT_TYPE) {
+               break;
+       case NE_FIT_TYPE:
                /*
                 * Split no edge of fit VA.
                 *
@@ -980,7 +984,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
                 * Shrink this VA to remaining size.
                 */
                va->va_start = nva_start_addr + size;
-       } else {
+               break;
+       default:
                return -1;
        }

       Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Uladzislau Rezki]

> 
> I managed to un-confuse gcc-8 by turning the if/else if/else into
> a switch statement. If you all think this is an acceptable solution,
> I'll submit that after some more testing to ensure it addresses
> all configurations:
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index a9213fc3802d..5b7e50de008b 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -915,7 +915,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>  {
>         struct vmap_area *lva;
> 
> -       if (type == FL_FIT_TYPE) {
> +       switch (type) {
> +       case FL_FIT_TYPE:
>                 /*
>                  * No need to split VA, it fully fits.
>                  *
> @@ -925,7 +926,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  */
>                 unlink_va(va, &free_vmap_area_root);
>                 kmem_cache_free(vmap_area_cachep, va);
> -       } else if (type == LE_FIT_TYPE) {
> +               break;
> +       case LE_FIT_TYPE:
>                 /*
>                  * Split left edge of fit VA.
>                  *
> @@ -934,7 +936,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  * |-------|-------|
>                  */
>                 va->va_start += size;
> -       } else if (type == RE_FIT_TYPE) {
> +               break;
> +       case RE_FIT_TYPE:
>                 /*
>                  * Split right edge of fit VA.
>                  *
> @@ -943,7 +946,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  * |-------|-------|
>                  */
>                 va->va_end = nva_start_addr;
> -       } else if (type == NE_FIT_TYPE) {
> +               break;
> +       case NE_FIT_TYPE:
>                 /*
>                  * Split no edge of fit VA.
>                  *
> @@ -980,7 +984,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
>                  * Shrink this VA to remaining size.
>                  */
>                 va->va_start = nva_start_addr + size;
> -       } else {
> +               break;
> +       default:
>                 return -1;
>         }
> 
To me it is not clear how it would solve the warning. It sounds like
your GCC after this change is able to keep track of that variable
probably because of less generated code. But i am not sure about
other versions. For example i have:

gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)

and it totally OK, i.e. it does not emit any related warning.

Another thing is that, if we add mode code there or change the function
prototype, we might run into the same warning. Therefore i proposed that
we just set the variable to NULL, i.e. Initialize it.

<snip>
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index b1bb5fc6eb05..10cfb93aba1e 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -913,7 +913,11 @@ adjust_va_to_fit_type(struct vmap_area *va,
        unsigned long nva_start_addr, unsigned long size,
        enum fit_type type)
 {
-       struct vmap_area *lva;
+       /*
+        * Some GCC versions can emit bogus warning that it
+        * may be used uninitialized, therefore set it NULL.
+        */
+       struct vmap_area *lva = NULL;
 
        if (type == FL_FIT_TYPE) {
                /*
<snip>

--
Vlad Rezki

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 6:57 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index a9213fc3802d..5b7e50de008b 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -915,7 +915,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >  {
> >         struct vmap_area *lva;
> >
> > -       if (type == FL_FIT_TYPE) {
> > +       switch (type) {
> > +       case FL_FIT_TYPE:
> >                 /*
> >                  * No need to split VA, it fully fits.
> >                  *
> > @@ -925,7 +926,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  */
> >                 unlink_va(va, &free_vmap_area_root);
> >                 kmem_cache_free(vmap_area_cachep, va);
> > -       } else if (type == LE_FIT_TYPE) {
> > +               break;
> > +       case LE_FIT_TYPE:
> >                 /*
> >                  * Split left edge of fit VA.
> >                  *
> > @@ -934,7 +936,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  * |-------|-------|
> >                  */
> >                 va->va_start += size;
> > -       } else if (type == RE_FIT_TYPE) {
> > +               break;
> > +       case RE_FIT_TYPE:
> >                 /*
> >                  * Split right edge of fit VA.
> >                  *
> > @@ -943,7 +946,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  * |-------|-------|
> >                  */
> >                 va->va_end = nva_start_addr;
> > -       } else if (type == NE_FIT_TYPE) {
> > +               break;
> > +       case NE_FIT_TYPE:
> >                 /*
> >                  * Split no edge of fit VA.
> >                  *
> > @@ -980,7 +984,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  * Shrink this VA to remaining size.
> >                  */
> >                 va->va_start = nva_start_addr + size;
> > -       } else {
> > +               break;
> > +       default:
> >                 return -1;
> >         }
> >
> To me it is not clear how it would solve the warning. It sounds like
> your GCC after this change is able to keep track of that variable
> probably because of less generated code. But i am not sure about
> other versions. For example i have:
>
> gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)
>
> and it totally OK, i.e. it does not emit any related warning.

To provide some background here, I'm doing randconfig tests, and
this warning might be one that only shows up with a specific combination
of options that add complexity to the build.

I do run into a lot -Wmaybe-uninitialized warnings, and most of the time
can figure out to change the code to be more readable by both
humans and compilers in a way that shuts up the warning. The
underlying algorithm in the compiler is NP-complete, so it can't
ever get it right 100%, but it is a valuable warning in general.

Using switch/case makes it easier for the compiler because it
seems to turn this into a single conditional instead of a set of
conditions. It also seems to be the much more common style
in the kernel.

> Another thing is that, if we add mode code there or change the function
> prototype, we might run into the same warning. Therefore i proposed that
> we just set the variable to NULL, i.e. Initialize it.

The problem with adding explicit NULL initializations is that this is
more likely to hide actual bugs if the code changes again, and the
compiler no longer notices the problem, so I try to avoid ever
initializing a variable to something that would cause a runtime
bug in place of a compile time warning later.

       Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 6:57 PM Uladzislau Rezki <urezki@gmail.com> wrote:
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index a9213fc3802d..5b7e50de008b 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -915,7 +915,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >  {
> >         struct vmap_area *lva;
> >
> > -       if (type == FL_FIT_TYPE) {
> > +       switch (type) {
> > +       case FL_FIT_TYPE:
> >                 /*
> >                  * No need to split VA, it fully fits.
> >                  *
> > @@ -925,7 +926,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  */
> >                 unlink_va(va, &free_vmap_area_root);
> >                 kmem_cache_free(vmap_area_cachep, va);
> > -       } else if (type == LE_FIT_TYPE) {
> > +               break;
> > +       case LE_FIT_TYPE:
> >                 /*
> >                  * Split left edge of fit VA.
> >                  *
> > @@ -934,7 +936,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  * |-------|-------|
> >                  */
> >                 va->va_start += size;
> > -       } else if (type == RE_FIT_TYPE) {
> > +               break;
> > +       case RE_FIT_TYPE:
> >                 /*
> >                  * Split right edge of fit VA.
> >                  *
> > @@ -943,7 +946,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  * |-------|-------|
> >                  */
> >                 va->va_end = nva_start_addr;
> > -       } else if (type == NE_FIT_TYPE) {
> > +               break;
> > +       case NE_FIT_TYPE:
> >                 /*
> >                  * Split no edge of fit VA.
> >                  *
> > @@ -980,7 +984,8 @@ adjust_va_to_fit_type(struct vmap_area *va,
> >                  * Shrink this VA to remaining size.
> >                  */
> >                 va->va_start = nva_start_addr + size;
> > -       } else {
> > +               break;
> > +       default:
> >                 return -1;
> >         }
> >
> To me it is not clear how it would solve the warning. It sounds like
> your GCC after this change is able to keep track of that variable
> probably because of less generated code. But i am not sure about
> other versions. For example i have:
>
> gcc version 6.3.0 20170516 (Debian 6.3.0-18+deb9u1)
>
> and it totally OK, i.e. it does not emit any related warning.

To provide some background here, I'm doing randconfig tests, and
this warning might be one that only shows up with a specific combination
of options that add complexity to the build.

I do run into a lot -Wmaybe-uninitialized warnings, and most of the time
can figure out to change the code to be more readable by both
humans and compilers in a way that shuts up the warning. The
underlying algorithm in the compiler is NP-complete, so it can't
ever get it right 100%, but it is a valuable warning in general.

Using switch/case makes it easier for the compiler because it
seems to turn this into a single conditional instead of a set of
conditions. It also seems to be the much more common style
in the kernel.

> Another thing is that, if we add mode code there or change the function
> prototype, we might run into the same warning. Therefore i proposed that
> we just set the variable to NULL, i.e. Initialize it.

The problem with adding explicit NULL initializations is that this is
more likely to hide actual bugs if the code changes again, and the
compiler no longer notices the problem, so I try to avoid ever
initializing a variable to something that would cause a runtime
bug in place of a compile time warning later.

       Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 9:29 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jun 17, 2019 at 6:57 PM Uladzislau Rezki <urezki@gmail.com> wrote:

> Using switch/case makes it easier for the compiler because it
> seems to turn this into a single conditional instead of a set of
> conditions. It also seems to be the much more common style
> in the kernel.

Nevermind, the warning came back after all. It's now down to
one out of 2000 randconfig builds I tested, but that's not good
enough. I'll send a patch the way you suggested.

      Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Arnd Bergmann]

On Mon, Jun 17, 2019 at 9:29 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Jun 17, 2019 at 6:57 PM Uladzislau Rezki <urezki@gmail.com> wrote:

> Using switch/case makes it easier for the compiler because it
> seems to turn this into a single conditional instead of a set of
> conditions. It also seems to be the much more common style
> in the kernel.

Nevermind, the warning came back after all. It's now down to
one out of 2000 randconfig builds I tested, but that's not good
enough. I'll send a patch the way you suggested.

      Arnd

Re: [BUG]: mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas

[Uladzislau Rezki]

Hello, Arnd.

> 
> Nevermind, the warning came back after all. It's now down to
> one out of 2000 randconfig builds I tested, but that's not good
> enough. I'll send a patch the way you suggested.
> 
Makes sense to me :)

Thank you.

--
Vlad Rezki