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 <[email protected]>
Signed-off-by: Johannes Weiner <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Johannes Weiner <[email protected]>
> ---
> 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))
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 <[email protected]>
> > Signed-off-by: Johannes Weiner <[email protected]>
> > ---
> > 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?
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 <[email protected]>
>>> Signed-off-by: Johannes Weiner <[email protected]>
>>> ---
>>> 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?
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: [email protected] [v6.5+]
Reported-by: Chengming Zhou <[email protected]> [Bug #1]
Reported-by: Jann Horn <[email protected]> [Bug #2]
Signed-off-by: Johannes Weiner <[email protected]>
---
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
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: [email protected] [v6.5+]
> Reported-by: Chengming Zhou <[email protected]> [Bug #1]
> Reported-by: Jann Horn <[email protected]> [Bug #2]
> Signed-off-by: Johannes Weiner <[email protected]>
Looks good to me.
Reviewed-by: Chengming Zhou <[email protected]>
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))
On Fri, Mar 15, 2024 at 4:55 PM Johannes Weiner <[email protected]> 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: [email protected] [v6.5+]
> Reported-by: Chengming Zhou <[email protected]> [Bug #1]
> Reported-by: Jann Horn <[email protected]> [Bug #2]
> Signed-off-by: Johannes Weiner <[email protected]>
Nice catch! Thanks for the report, Chengming and Jann, and thanks for
the fix, Johannes!
Reviewed-by: Nhat Pham <[email protected]>
> ---
> 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
>