Re: [PATCH] mm/vmalloc: move 'area->pages' after if statement
2019년 8월 31일 (토) 오전 5:50, Andrew Morton 님이 작성: > > On Fri, 30 Aug 2019 12:57:16 +0900 Austin Kim wrote: > > > If !area->pages statement is true where memory allocation fails, > > area is freed. > > > > In this case 'area->pages = pages' should not executed. > > So move 'area->pages = pages' after if statement. > > > > ... > > > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -2416,13 +2416,15 @@ static void *__vmalloc_area_node(struct vm_struct > > *area, gfp_t gfp_mask, > > } else { > > pages = kmalloc_node(array_size, nested_gfp, node); > > } > > - area->pages = pages; > > - if (!area->pages) { > > + > > + if (!pages) { > > remove_vm_area(area->addr); > > kfree(area); > > return NULL; > > } > > > > + area->pages = pages; > > + > > for (i = 0; i < area->nr_pages; i++) { > > struct page *page; > > > > Fair enough. But we can/should also do this? I agreed since it is the same treatment. Thanks for feedback. > > --- a/mm/vmalloc.c~mm-vmalloc-move-area-pages-after-if-statement-fix > +++ a/mm/vmalloc.c > @@ -2409,7 +2409,6 @@ static void *__vmalloc_area_node(struct > nr_pages = get_vm_area_size(area) >> PAGE_SHIFT; > array_size = (nr_pages * sizeof(struct page *)); > > - area->nr_pages = nr_pages; > /* Please note that the recursion is strictly bounded. */ > if (array_size > PAGE_SIZE) { > pages = __vmalloc_node(array_size, 1, nested_gfp|highmem_mask, > @@ -2425,6 +2424,7 @@ static void *__vmalloc_area_node(struct > } > > area->pages = pages; > + area->nr_pages = nr_pages; > > for (i = 0; i < area->nr_pages; i++) { > struct page *page; > _ >
Re: [PATCH] mm/vmalloc: move 'area->pages' after if statement
On 8/30/19 10:50 PM, Andrew Morton wrote: > On Fri, 30 Aug 2019 12:57:16 +0900 Austin Kim wrote: > >> If !area->pages statement is true where memory allocation fails, >> area is freed. >> >> In this case 'area->pages = pages' should not executed. That read like a use after free fix and made me wonder about stable, fixes etc... luckily it's not. >> So move 'area->pages = pages' after if statement. >> >> ... >> >> --- a/mm/vmalloc.c >> +++ b/mm/vmalloc.c >> @@ -2416,13 +2416,15 @@ static void *__vmalloc_area_node(struct vm_struct >> *area, gfp_t gfp_mask, >> } else { >> pages = kmalloc_node(array_size, nested_gfp, node); >> } >> -area->pages = pages; >> -if (!area->pages) { >> + >> +if (!pages) { >> remove_vm_area(area->addr); >> kfree(area); >> return NULL; >> } >> >> +area->pages = pages; >> + >> for (i = 0; i < area->nr_pages; i++) { >> struct page *page; >> > > Fair enough. But we can/should also do this? Agreed, same thing. Acked-by: Vlastimil Babka > --- a/mm/vmalloc.c~mm-vmalloc-move-area-pages-after-if-statement-fix > +++ a/mm/vmalloc.c > @@ -2409,7 +2409,6 @@ static void *__vmalloc_area_node(struct > nr_pages = get_vm_area_size(area) >> PAGE_SHIFT; > array_size = (nr_pages * sizeof(struct page *)); > > - area->nr_pages = nr_pages; > /* Please note that the recursion is strictly bounded. */ > if (array_size > PAGE_SIZE) { > pages = __vmalloc_node(array_size, 1, nested_gfp|highmem_mask, > @@ -2425,6 +2424,7 @@ static void *__vmalloc_area_node(struct > } > > area->pages = pages; > + area->nr_pages = nr_pages; > > for (i = 0; i < area->nr_pages; i++) { > struct page *page; > _ > >
Re: [PATCH] mm/vmalloc: move 'area->pages' after if statement
On Fri, 30 Aug 2019 12:57:16 +0900 Austin Kim wrote: > If !area->pages statement is true where memory allocation fails, > area is freed. > > In this case 'area->pages = pages' should not executed. > So move 'area->pages = pages' after if statement. > > ... > > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2416,13 +2416,15 @@ static void *__vmalloc_area_node(struct vm_struct > *area, gfp_t gfp_mask, > } else { > pages = kmalloc_node(array_size, nested_gfp, node); > } > - area->pages = pages; > - if (!area->pages) { > + > + if (!pages) { > remove_vm_area(area->addr); > kfree(area); > return NULL; > } > > + area->pages = pages; > + > for (i = 0; i < area->nr_pages; i++) { > struct page *page; > Fair enough. But we can/should also do this? --- a/mm/vmalloc.c~mm-vmalloc-move-area-pages-after-if-statement-fix +++ a/mm/vmalloc.c @@ -2409,7 +2409,6 @@ static void *__vmalloc_area_node(struct nr_pages = get_vm_area_size(area) >> PAGE_SHIFT; array_size = (nr_pages * sizeof(struct page *)); - area->nr_pages = nr_pages; /* Please note that the recursion is strictly bounded. */ if (array_size > PAGE_SIZE) { pages = __vmalloc_node(array_size, 1, nested_gfp|highmem_mask, @@ -2425,6 +2424,7 @@ static void *__vmalloc_area_node(struct } area->pages = pages; + area->nr_pages = nr_pages; for (i = 0; i < area->nr_pages; i++) { struct page *page; _
Re: [PATCH] mm/vmalloc: move 'area->pages' after if statement
On Fri 30-08-19 12:57:16, Austin Kim wrote: > If !area->pages statement is true where memory allocation fails, > area is freed. > > In this case 'area->pages = pages' should not executed. > So move 'area->pages = pages' after if statement. > > Signed-off-by: Austin Kim Acked-by: Michal Hocko Thanks! > --- > mm/vmalloc.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index b810103..af93ba6 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2416,13 +2416,15 @@ static void *__vmalloc_area_node(struct vm_struct > *area, gfp_t gfp_mask, > } else { > pages = kmalloc_node(array_size, nested_gfp, node); > } > - area->pages = pages; > - if (!area->pages) { > + > + if (!pages) { > remove_vm_area(area->addr); > kfree(area); > return NULL; > } > > + area->pages = pages; > + > for (i = 0; i < area->nr_pages; i++) { > struct page *page; > > -- > 2.6.2 > -- Michal Hocko SUSE Labs
[PATCH] mm/vmalloc: move 'area->pages' after if statement
If !area->pages statement is true where memory allocation fails, area is freed. In this case 'area->pages = pages' should not executed. So move 'area->pages = pages' after if statement. Signed-off-by: Austin Kim --- mm/vmalloc.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mm/vmalloc.c b/mm/vmalloc.c index b810103..af93ba6 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2416,13 +2416,15 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask, } else { pages = kmalloc_node(array_size, nested_gfp, node); } - area->pages = pages; - if (!area->pages) { + + if (!pages) { remove_vm_area(area->addr); kfree(area); return NULL; } + area->pages = pages; + for (i = 0; i < area->nr_pages; i++) { struct page *page; -- 2.6.2