Received-SPF: pass (google.com: domain of linux-kernel+bounces-105036-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240314164941.580454-1-hannes@cmpxchg.org> <1551fa14-2a95-49fd-ab1a-11c38ae29486@linux.dev>
 <20240315093010.GB581298@cmpxchg.org> <ae197190-6a15-49c5-ab3c-3eaac6dd4c5c@linux.dev>
 <20240315095556.GC581298@cmpxchg.org>
In-Reply-To: <20240315095556.GC581298@cmpxchg.org>
From: Nhat Pham <nphamcs@gmail.com>
Date: Sat, 16 Mar 2024 09:41:04 +0700
Message-ID: <CAKEwX=OH=057RRGS0wv9tMKJK6+w8RMQa8neTPW1wfhpEmuZLw@mail.gmail.com>
Subject: Re: [PATCH] mm: cachestat: fix two shmem bugs
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Chengming Zhou <chengming.zhou@linux.dev>, Andrew Morton <akpm@linux-foundation.org>, 
	linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	Jann Horn <jannh@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 15, 2024 at 4:55=E2=80=AFPM Johannes Weiner <hannes@cmpxchg.org=
> wrote:
>
> When cachestat on shmem races with swapping and invalidation, there
> are two possible bugs:
>
> 1) A swapin error can have resulted in a poisoned swap entry in the
>    shmem inode's xarray. Calling get_shadow_from_swap_cache() on it
>    will result in an out-of-bounds access to swapper_spaces[].
>
>    Validate the entry with non_swap_entry() before going further.
>
> 2) When we find a valid swap entry in the shmem's inode, the shadow
>    entry in the swapcache might not exist yet: swap IO is still in
>    progress and we're before __remove_mapping; swapin, invalidation,
>    or swapoff have removed the shadow from swapcache after we saw the
>    shmem swap entry.
>
>    This will send a NULL to workingset_test_recent(). The latter
>    purely operates on pointer bits, so it won't crash - node 0, memcg
>    ID 0, eviction timestamp 0, etc. are all valid inputs - but it's a
>    bogus test. In theory that could result in a false "recently
>    evicted" count.
>
>    Such a false positive wouldn't be the end of the world. But for
>    code clarity and (future) robustness, be explicit about this case.
>
>    Bail on get_shadow_from_swap_cache() returning NULL.
>
> Fixes: cf264e1329fb ("cachestat: implement cachestat syscall")
> Cc: stable@vger.kernel.org                              [v6.5+]
> Reported-by: Chengming Zhou <chengming.zhou@linux.dev>  [Bug #1]
> Reported-by: Jann Horn <jannh@google.com>               [Bug #2]
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Nice catch! Thanks for the report, Chengming and Jann, and thanks for
the fix, Johannes!
Reviewed-by: Nhat Pham <nphamcs@gmail.com>

> ---
>  mm/filemap.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 222adac7c9c5..0aa91bf6c1f7 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -4198,7 +4198,23 @@ static void filemap_cachestat(struct address_space=
 *mapping,
>                                 /* shmem file - in swap cache */
>                                 swp_entry_t swp =3D radix_to_swp_entry(fo=
lio);
>
> +                               /* swapin error results in poisoned entry=
 */
> +                               if (non_swap_entry(swp))
> +                                       goto resched;
> +
> +                               /*
> +                                * Getting a swap entry from the shmem
> +                                * inode means we beat
> +                                * shmem_unuse(). rcu_read_lock()
> +                                * ensures swapoff waits for us before
> +                                * freeing the swapper space. However,
> +                                * we can race with swapping and
> +                                * invalidation, so there might not be
> +                                * a shadow in the swapcache (yet).
> +                                */
>                                 shadow =3D get_shadow_from_swap_cache(swp=
);
> +                               if (!shadow)
> +                                       goto resched;
>                         }
>  #endif
>                         if (workingset_test_recent(shadow, true, &working=
set))
> --
> 2.44.0
>