[v2] mm/shmem: fix THP allocation and fallback loop

[PATCH v2] mm/shmem: fix THP allocation and fallback loop

Posted by Kairui Song 3 months, 2 weeks ago

From: Kairui Song <kasong@tencent.com>

The order check and fallback loop is updating the index value on
every loop, this will cause the index to be aligned by a larger
value while the loop shrinks the order.

This may result in inserting and returning a folio of the wrong index
and cause data corruption with some userspace workloads [1].

Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/linux-mm/CAMgjq7DqgAmj25nDUwwu1U2cSGSn8n4-Hqpgottedy0S6YYeUw@mail.gmail.com/ [1]
Fixes: e7a2ab7b3bb5d ("mm: shmem: add mTHP support for anonymous shmem")
Signed-off-by: Kairui Song <kasong@tencent.com>
---

Changes from V1:
- Link to V1: https://lore.kernel.org/linux-mm/20251021190436.81682-1-ryncsn@gmail.com/
- Remove unnecessary cleanup and simplify the commit message.

 mm/shmem.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index b50ce7dbc84a..7559773ebb30 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1895,10 +1895,11 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
 		order = highest_order(suitable_orders);
 		while (suitable_orders) {
 			pages = 1UL << order;
-			index = round_down(index, pages);
-			folio = shmem_alloc_folio(gfp, order, info, index);
-			if (folio)
+			folio = shmem_alloc_folio(gfp, order, info, round_down(index, pages));
+			if (folio) {
+				index = round_down(index, pages);
 				goto allocated;
+			}
 
 			if (pages == HPAGE_PMD_NR)
 				count_vm_event(THP_FILE_FALLBACK);
-- 
2.51.0

Re: [PATCH v2] mm/shmem: fix THP allocation and fallback loop

Posted by Barry Song 3 months, 2 weeks ago

>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index b50ce7dbc84a..7559773ebb30 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1895,10 +1895,11 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
>                 order = highest_order(suitable_orders);
>                 while (suitable_orders) {
>                         pages = 1UL << order;
> -                       index = round_down(index, pages);
> -                       folio = shmem_alloc_folio(gfp, order, info, index);
> -                       if (folio)
> +                       folio = shmem_alloc_folio(gfp, order, info, round_down(index, pages));
> +                       if (folio) {
> +                               index = round_down(index, pages);
>                                 goto allocated;
> +                       }

Could this be a temporary variable to store round_down(index, pages)?

>
>                         if (pages == HPAGE_PMD_NR)
>                                 count_vm_event(THP_FILE_FALLBACK);
> --

Thanks
Barry

Re: [PATCH v2] mm/shmem: fix THP allocation and fallback loop

Posted by Kairui Song 3 months, 2 weeks ago

On Thu, Oct 23, 2025 at 1:51 PM Barry Song <21cnbao@gmail.com> wrote:
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index b50ce7dbc84a..7559773ebb30 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1895,10 +1895,11 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> >                 order = highest_order(suitable_orders);
> >                 while (suitable_orders) {
> >                         pages = 1UL << order;
> > -                       index = round_down(index, pages);
> > -                       folio = shmem_alloc_folio(gfp, order, info, index);
> > -                       if (folio)
> > +                       folio = shmem_alloc_folio(gfp, order, info, round_down(index, pages));
> > +                       if (folio) {
> > +                               index = round_down(index, pages);
> >                                 goto allocated;
> > +                       }
>
> Could this be a temporary variable to store round_down(index, pages)?

Right we can do that, but the generated code should be the same, the
compiler is smart enough, I just checked the generated code with gcc /
clang.

Do you think the code will be cleaner with a temporary variable? I can
send a V3 if anyone suggests, it's really a trivial change.

Re: [PATCH v2] mm/shmem: fix THP allocation and fallback loop

Posted by Barry Song 3 months, 2 weeks ago

On Thu, Oct 23, 2025 at 6:58 PM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Thu, Oct 23, 2025 at 1:51 PM Barry Song <21cnbao@gmail.com> wrote:
> > > diff --git a/mm/shmem.c b/mm/shmem.c
> > > index b50ce7dbc84a..7559773ebb30 100644
> > > --- a/mm/shmem.c
> > > +++ b/mm/shmem.c
> > > @@ -1895,10 +1895,11 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf,
> > >                 order = highest_order(suitable_orders);
> > >                 while (suitable_orders) {
> > >                         pages = 1UL << order;
> > > -                       index = round_down(index, pages);
> > > -                       folio = shmem_alloc_folio(gfp, order, info, index);
> > > -                       if (folio)
> > > +                       folio = shmem_alloc_folio(gfp, order, info, round_down(index, pages));
> > > +                       if (folio) {
> > > +                               index = round_down(index, pages);
> > >                                 goto allocated;
> > > +                       }
> >
> > Could this be a temporary variable to store round_down(index, pages)?
>
> Right we can do that, but the generated code should be the same, the
> compiler is smart enough, I just checked the generated code with gcc /
> clang.
>
> Do you think the code will be cleaner with a temporary variable? I can
> send a V3 if anyone suggests, it's really a trivial change.

Personally, I think a temporary variable makes the code cleaner, but I don’t
have any strong preference.

The fix looks correct to me.

Thanks
Barry