Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230115133330.28420-1-msizanoen@qtmlabs.xyz> <20230115134651.30028-1-msizanoen@qtmlabs.xyz>
In-Reply-To: <20230115134651.30028-1-msizanoen@qtmlabs.xyz>
From:   Yu Zhao <yuzhao@google.com>
Date:   Sun, 15 Jan 2023 16:13:44 -0700
Message-ID: <CAOUHufahcS0G_GApTdmzE4_Nb_70LGaCkgV0NR_xJuWN2NdJVg@mail.gmail.com>
Subject: Re: [PATCH v2] mm: do not try to migrate lru_gen if it's not
 associated with a memcg
To:     msizanoen1 <msizanoen@qtmlabs.xyz>
Cc:     Andrew Morton <akpm@linux-foundation.org>, stable@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Jan 15, 2023 at 6:47 AM msizanoen1 <msizanoen@qtmlabs.xyz> wrote:
>
> In some cases, memory cgroup migration can be initiated by userspace
> right after a process was created and right before `lru_gen_add_mm()` is
> called (e.g. by some program watching a cgroup and moving away any
> processes it detects[1]), which results in the following sequence of
> WARNs followed by an Oops as the kernel attempts to perform a
> `lru_gen_add_mm()` twice on the same `mm`:

...

> Fix this by simply leaving the lru_gen alone if it has not been
> associated with a memcg yet, as it should eventually be assigned to the
> right cgroup anyway.
>
> [1]: https://gitlab.freedesktop.org/benzea/uresourced/-/blob/master/cgroupify/cgroupify.c
>
> v2:
>         Added stable cc tags
>
> Signed-off-by: N/A (patch should not be copyrightable)
> Cc: stable@vger.kernel.org

Thanks for the fix.  Cc'ing stable is the right thing to do. The
commit message and the comment styles could be easily adjusted to
align with the guidelines.

I don't think the N/A is acceptible though. I fully respect it if you
wish to remain anonymous -- I can send a similar fix crediting you
as the "anonymous user <msizanoen@qtmlabs.xyz>" who reported this bug.

A bit of background on how I broke it: an old version I have on 4.15
calls lru_gen_add_mm() before cgroup_post_fork(), which excludes
cgroup migrations by cgroup_threadgroup_rwsem. When I rebased it, I
made lru_gen_add_mm() depend on task_lock for the synchronization with
cgroup migrations -- the decoupling seemed (still seems) to make it
less complicated -- but this is not safe unless we have the check below.


> ---
>  mm/vmscan.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bd6637fcd8f9..0cac40e7484c 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3323,13 +3323,19 @@ void lru_gen_migrate_mm(struct mm_struct *mm)
>         if (mem_cgroup_disabled())
>                 return;
>
> +       /* This could happen if cgroup migration is invoked before the process
> +        * lru_gen is associated with a memcg (e.g. during process creation).
> +        * Simply ignore it in this case as the lru_gen will get assigned the
> +        * right cgroup later. */
> +       if (!mm->lru_gen.memcg)
> +               return;
> +
>         rcu_read_lock();
>         memcg = mem_cgroup_from_task(task);
>         rcu_read_unlock();
>         if (memcg == mm->lru_gen.memcg)
>                 return;
>
> -       VM_WARN_ON_ONCE(!mm->lru_gen.memcg);
>         VM_WARN_ON_ONCE(list_empty(&mm->lru_gen.list));
>
>         lru_gen_del_mm(mm);