Re: [syzbot] general protection fault in mod_memcg_page_state

[Shakeel Butt]

On Thu, Dec 30, 2021 at 12:53 AM Michal Hocko <mhocko@xxxxxxxx> wrote:
[...]
>
> This might have something to do with http://lkml.kernel.org/r/20211222052457.1960701-1-shakeelb@xxxxxxxxxx
> which has added the accounting which is blowing up. The problem happens
> when a memcg is retrieved from the allocated page. This should be NULL
> as the reported commit doesn't really add any __GFP_ACCOUNT user AFAICS.
> Anyway vm_area_alloc_pages can fail the allocation if the current
> context has fatal signals pending. array->pages array is allocated with
> __GFP_ZERO so the failed allocation should have kept the pages[0] NULL.
> I haven't followed the page->memcg path to double check whether that
> could lead to 0xdffffc0000000001 in the end.
>
> I believe we need something like
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 9bf838817a47..d2e392cac909 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2627,7 +2627,8 @@ static void __vunmap(const void *addr, int deallocate_pages)
>                 unsigned int page_order = vm_area_page_order(area);
>                 int i;
>
> -               mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC,
> +               if (area->pages[0])
> +                       mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC,
>                                      -area->nr_pages);
>
>                 for (i = 0; i < area->nr_pages; i += 1U << page_order) {
> @@ -2968,7 +2969,8 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>                 page_order, nr_small_pages, area->pages);
>
>         atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
> -       mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC, area->nr_pages);
> +       if (area->pages[0])
> +               mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC, area->nr_pages);
>
>         /*
>          * If not enough pages were obtained to accomplish an
>
> Or to account each page separately so that we do not have to rely on
> pages[0].
>

Thanks Michal for the CC. I will add these checks in the next version
(or convert to account each page separately based on discussion on the
other thread).

Shakeel