[PATCH] mm: cachestat: avoid bogus workingset test during swapping & invalidation races

[PATCH] mm: cachestat: avoid bogus workingset test during swapping & invalidation races

[Johannes Weiner]

When cachestat against shmem races with swapping and invalidation, the
shadow entry might not exist: swapout IO is still in progress and
we're before __remove_mapping; or swapin/invalidation/swapoff has
removed the shadow from swapcache after we saw a 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.

Fixes: cf264e1329fb ("cachestat: implement cachestat syscall")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/filemap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index 222adac7c9c5..a07c27df7eab 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -4199,6 +4199,9 @@ static void filemap_cachestat(struct address_space *mapping,
 				swp_entry_t swp = radix_to_swp_entry(folio);
 
 				shadow = get_shadow_from_swap_cache(swp);
+				/* can race with swapping & invalidation */
+				if (!shadow)
+					goto resched;
 			}
 #endif
 			if (workingset_test_recent(shadow, true, &workingset))
-- 
2.44.0

Re: [PATCH] mm: cachestat: avoid bogus workingset test during swapping & invalidation races

[Chengming Zhou]

On 2024/3/15 00:49, Johannes Weiner wrote:
> When cachestat against shmem races with swapping and invalidation, the
> shadow entry might not exist: swapout IO is still in progress and
> we're before __remove_mapping; or swapin/invalidation/swapoff has
> removed the shadow from swapcache after we saw a 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.

Good catch!

> 
> Such a false positive wouldn't be the end of the world. But for code
> clarity and (future) robustness, be explicit about this case.
> 
> Fixes: cf264e1329fb ("cachestat: implement cachestat syscall")
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/filemap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 222adac7c9c5..a07c27df7eab 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -4199,6 +4199,9 @@ static void filemap_cachestat(struct address_space *mapping,
>  				swp_entry_t swp = radix_to_swp_entry(folio);
>  

IIUC, we should first check if it's a real swap entry using non_swap_entry(), right?
Since there maybe other types of entries in shmem. And need to get_swap_device() to
prevent concurrent swapoff here, get_shadow_from_swap_cache() won't do it for us.

Thanks.

>  				shadow = get_shadow_from_swap_cache(swp);
> +				/* can race with swapping & invalidation */
> +				if (!shadow)
> +					goto resched;
>  			}
>  #endif
>  			if (workingset_test_recent(shadow, true, &workingset))

Re: [PATCH] mm: cachestat: avoid bogus workingset test during swapping & invalidation races

[Johannes Weiner]

On Fri, Mar 15, 2024 at 11:16:35AM +0800, Chengming Zhou wrote:
> On 2024/3/15 00:49, Johannes Weiner wrote:
> > When cachestat against shmem races with swapping and invalidation, the
> > shadow entry might not exist: swapout IO is still in progress and
> > we're before __remove_mapping; or swapin/invalidation/swapoff has
> > removed the shadow from swapcache after we saw a 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.
> 
> Good catch!
> 
> > 
> > Such a false positive wouldn't be the end of the world. But for code
> > clarity and (future) robustness, be explicit about this case.
> > 
> > Fixes: cf264e1329fb ("cachestat: implement cachestat syscall")
> > Reported-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > ---
> >  mm/filemap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 222adac7c9c5..a07c27df7eab 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -4199,6 +4199,9 @@ static void filemap_cachestat(struct address_space *mapping,
> >  				swp_entry_t swp = radix_to_swp_entry(folio);
> >  
> 
> IIUC, we should first check if it's a real swap entry using non_swap_entry(), right?
> Since there maybe other types of entries in shmem.

Good point, it could be a poisoned entry. I'll add the
non_swap_entry() check on swp.

> And need to get_swap_device() to prevent concurrent swapoff here,
> get_shadow_from_swap_cache() won't do it for us.

We're holding rcu_read_lock() for the xarray iteration, so if we see
the swap entry in the shmem mapping, it means we beat shmem_unuse()
and swapoff hasn't run synchronize_rcu() yet.

So it's safe. But I think it could use a comment. Maybe the
documentation of get_swap_device() should mention this option too?

Re: [PATCH] mm: cachestat: avoid bogus workingset test during swapping & invalidation races

[Chengming Zhou]

On 2024/3/15 17:30, Johannes Weiner wrote:
> On Fri, Mar 15, 2024 at 11:16:35AM +0800, Chengming Zhou wrote:
>> On 2024/3/15 00:49, Johannes Weiner wrote:
>>> When cachestat against shmem races with swapping and invalidation, the
>>> shadow entry might not exist: swapout IO is still in progress and
>>> we're before __remove_mapping; or swapin/invalidation/swapoff has
>>> removed the shadow from swapcache after we saw a 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.
>>
>> Good catch!
>>
>>>
>>> Such a false positive wouldn't be the end of the world. But for code
>>> clarity and (future) robustness, be explicit about this case.
>>>
>>> Fixes: cf264e1329fb ("cachestat: implement cachestat syscall")
>>> Reported-by: Jann Horn <jannh@google.com>
>>> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>>> ---
>>>  mm/filemap.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/mm/filemap.c b/mm/filemap.c
>>> index 222adac7c9c5..a07c27df7eab 100644
>>> --- a/mm/filemap.c
>>> +++ b/mm/filemap.c
>>> @@ -4199,6 +4199,9 @@ static void filemap_cachestat(struct address_space *mapping,
>>>  				swp_entry_t swp = radix_to_swp_entry(folio);
>>>  
>>
>> IIUC, we should first check if it's a real swap entry using non_swap_entry(), right?
>> Since there maybe other types of entries in shmem.
> 
> Good point, it could be a poisoned entry. I'll add the
> non_swap_entry() check on swp.
> 
>> And need to get_swap_device() to prevent concurrent swapoff here,
>> get_shadow_from_swap_cache() won't do it for us.
> 
> We're holding rcu_read_lock() for the xarray iteration, so if we see
> the swap entry in the shmem mapping, it means we beat shmem_unuse()
> and swapoff hasn't run synchronize_rcu() yet.

Ah, you are right, so it's safe.

> 
> So it's safe. But I think it could use a comment. Maybe the
> documentation of get_swap_device() should mention this option too?

[PATCH] mm: cachestat: fix two shmem bugs

[Johannes Weiner]

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>
---
 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 = radix_to_swp_entry(folio);
 
+				/* 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 = get_shadow_from_swap_cache(swp);
+				if (!shadow)
+					goto resched;
 			}
 #endif
 			if (workingset_test_recent(shadow, true, &workingset))
-- 
2.44.0

Re: [PATCH] mm: cachestat: fix two shmem bugs

[Chengming Zhou]

On 2024/3/15 17:55, Johannes Weiner 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>

Looks good to me.

Reviewed-by: Chengming Zhou <chengming.zhou@linux.dev>

Thanks.

> ---
>  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 = radix_to_swp_entry(folio);
>  
> +				/* 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 = get_shadow_from_swap_cache(swp);
> +				if (!shadow)
> +					goto resched;
>  			}
>  #endif
>  			if (workingset_test_recent(shadow, true, &workingset))

Re: [PATCH] mm: cachestat: fix two shmem bugs

[Nhat Pham]

On Fri, Mar 15, 2024 at 4:55 PM 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 = radix_to_swp_entry(folio);
>
> +                               /* 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 = get_shadow_from_swap_cache(swp);
> +                               if (!shadow)
> +                                       goto resched;
>                         }
>  #endif
>                         if (workingset_test_recent(shadow, true, &workingset))
> --
> 2.44.0
>

Re: [PATCH] mm: cachestat: fix two shmem bugs

[Nhat Pham]

[-- Attachment #1: Type: text/plain, Size: 154 bytes --]

💖

Nhat Pham reacted via Gmail
<https://www.google.com/gmail/about/?utm_source=gmail-in-product&utm_medium=et&utm_campaign=emojireactionemail#app>

[-- Attachment #2: Type: text/vnd.google.email-reaction+json, Size: 40 bytes --]

{
  "emoji": "💖",
  "version": 1
}

[-- Attachment #3: Type: text/html, Size: 286 bytes --]