| Message ID | 20190617121427.77565-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [BUG] : mm/vmalloc: uninitialized variable access in pcpu_get_vm_areas | expand |
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
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
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
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
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
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
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
> > 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
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
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
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
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; }
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(-) -- 2.20.0