MIME-Version: 1.0
References: <00000000000049f33f05d4535526@google.com> <Yc1zbYqVO/6b6Uhf@dhcp22.suse.cz>
In-Reply-To: <Yc1zbYqVO/6b6Uhf@dhcp22.suse.cz>
From:   Shakeel Butt <shakeelb@google.com>
Date:   Thu, 30 Dec 2021 11:08:12 -0800
Message-ID: <CALvZod6S+zLw=mRw6F7g4+WSnCVaj+jHs7rjAoyDfFK92wq9jw@mail.gmail.com>
Subject: Re: [syzbot] general protection fault in mod_memcg_page_state
To:     Michal Hocko <mhocko@suse.com>
Cc:     syzbot <syzbot+864849a13d44b22de04d@syzkaller.appspotmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>, bpf <bpf@vger.kernel.org>,
        changbin.du@intel.com, Daniel Borkmann <daniel@iogearbox.net>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        hkallweit1@gmail.com, John Fastabend <john.fastabend@gmail.com>,
        Martin KaFai Lau <kafai@fb.com>, kpsingh@kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>,
        Song Liu <songliubraving@fb.com>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        yajun.deng@linux.dev, Yonghong Song <yhs@fb.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 30, 2021 at 12:53 AM Michal Hocko <mhocko@suse.com> wrote:
[...]
>
> This might have something to do with http://lkml.kernel.org/r/20211222052457.1960701-1-shakeelb@google.com
> 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