From: Pankaj Raghav <p.raghav@samsung.com>

split_huge_page_to_list_[to_order](), split_huge_page() and
try_folio_split() return 0 on success and error codes on failure.

When THP is disabled, these functions return 0 indicating success even
though an error code should be returned as it is not possible to split a
folio when THP is disabled.

Make all these functions return -EINVAL to indicate failure instead of
0.

This issue was discovered while experimenting enabling large folios
without THP and found that returning 0 in these functions is resulting in
undefined behavior in truncate operations. This change fixes the issue.

Fixes: c010d47f107f ("mm: thp: split huge page to any lower order pages")
Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 include/linux/huge_mm.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 29ef70022da1..48c4f91c5b13 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -588,22 +588,22 @@ static inline int
 split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
 		unsigned int new_order)
 {
-	return 0;
+	return -EINVAL;
 }
 static inline int split_huge_page(struct page *page)
 {
-	return 0;
+	return -EINVAL;
 }
 
 static inline int split_folio_to_list(struct folio *folio, struct list_head *list)
 {
-	return 0;
+	return -EINVAL;
 }
 
 static inline int try_folio_split(struct folio *folio, struct page *page,
 		struct list_head *list)
 {
-	return 0;
+	return -EINVAL;
 }
 
 static inline void deferred_split_folio(struct folio *folio, bool partially_mapped) {}

base-commit: 291634ccfd2820c09f6e8c4982c2dee8155d09ae
-- 
2.50.1

On Tue, Sep 02, 2025 at 10:40:36AM +0200, Pankaj Raghav (Samsung) wrote:
> From: Pankaj Raghav <p.raghav@samsung.com>
> 
> split_huge_page_to_list_[to_order](), split_huge_page() and
> try_folio_split() return 0 on success and error codes on failure.
> 
> When THP is disabled, these functions return 0 indicating success even
> though an error code should be returned as it is not possible to split a
> folio when THP is disabled.

Other view is that the page is already split therefore nop.

> Make all these functions return -EINVAL to indicate failure instead of
> 0.
> 
> This issue was discovered while experimenting enabling large folios
> without THP and found that returning 0 in these functions is resulting in
> undefined behavior in truncate operations. This change fixes the issue.

Could you elaborate on the undefined behaviour? I don't see it.

If you argue that this code should not be reachable on !THP config, add
WARN() there. But I don't see a value.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

On 9/2/25 13:22, Kiryl Shutsemau wrote:
> On Tue, Sep 02, 2025 at 10:40:36AM +0200, Pankaj Raghav (Samsung) wrote:
>> From: Pankaj Raghav <p.raghav@samsung.com>
>>
>> split_huge_page_to_list_[to_order](), split_huge_page() and
>> try_folio_split() return 0 on success and error codes on failure.
>>
>> When THP is disabled, these functions return 0 indicating success even
>> though an error code should be returned as it is not possible to split a
>> folio when THP is disabled.
> 
> Other view is that the page is already split therefore nop.
> 
>> Make all these functions return -EINVAL to indicate failure instead of
>> 0.
>>
>> This issue was discovered while experimenting enabling large folios
>> without THP and found that returning 0 in these functions is resulting in
>> undefined behavior in truncate operations. This change fixes the issue.
> 
> Could you elaborate on the undefined behaviour? I don't see it.
> 
> If you argue that this code should not be reachable on !THP config, add
> WARN() there. But I don't see a value.

Little bit of context:

I started investigating what it takes to remove large folio dependency on THP[1][2]

I have some non-upstream changes which enables Large block size (therefore it uses large folios) on
systems with !CONFIG_THP.

I was hitting a weird stale content read error and finally ended up with this fix.

I thought this is a self-contained patch that can already be upstream. My argument is not that this
should not be reachable, but returning -EINVAL will do the right thing instead of returning 0, which
means success.

I hope it clarifies a bit. Let me know what you think.

[1] https://lore.kernel.org/linux-mm/731d8b44-1a45-40bc-a274-8f39a7ae0f7f@lucifer.local/
[2] https://lore.kernel.org/all/aGfNKGBz9lhuK1AF@casper.infradead.org/
--
Pankaj

On Tue, Sep 02, 2025 at 02:15:42PM +0200, Pankaj Raghav wrote:
> 
> On 9/2/25 13:22, Kiryl Shutsemau wrote:
> > On Tue, Sep 02, 2025 at 10:40:36AM +0200, Pankaj Raghav (Samsung) wrote:
> >> From: Pankaj Raghav <p.raghav@samsung.com>
> >>
> >> split_huge_page_to_list_[to_order](), split_huge_page() and
> >> try_folio_split() return 0 on success and error codes on failure.
> >>
> >> When THP is disabled, these functions return 0 indicating success even
> >> though an error code should be returned as it is not possible to split a
> >> folio when THP is disabled.
> > 
> > Other view is that the page is already split therefore nop.
> > 
> >> Make all these functions return -EINVAL to indicate failure instead of
> >> 0.
> >>
> >> This issue was discovered while experimenting enabling large folios
> >> without THP and found that returning 0 in these functions is resulting in
> >> undefined behavior in truncate operations. This change fixes the issue.
> > 
> > Could you elaborate on the undefined behaviour? I don't see it.
> > 
> > If you argue that this code should not be reachable on !THP config, add
> > WARN() there. But I don't see a value.
> 
> Little bit of context:
> 
> I started investigating what it takes to remove large folio dependency on THP[1][2]
> 
> I have some non-upstream changes which enables Large block size (therefore it uses large folios) on
> systems with !CONFIG_THP.
> 
> I was hitting a weird stale content read error and finally ended up with this fix.
> 
> I thought this is a self-contained patch that can already be upstream. My argument is not that this
> should not be reachable, but returning -EINVAL will do the right thing instead of returning 0, which
> means success.

Okay, makes sense.

In THP=y case, __folio_split() also returns -EINVAL for !large folios,
but it is not very explicit:

	if (new_order >= folio_order(folio))
		return -EINVAL;

In THP=y, we also issue warning:

	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);

Makes sense to do the same here for THP=n. It might help to catch cases
we do not see with THP=y, like getting non-THP large folios here.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

>> I was hitting a weird stale content read error and finally ended up with this fix.
>>
>> I thought this is a self-contained patch that can already be upstream. My argument is not that this
>> should not be reachable, but returning -EINVAL will do the right thing instead of returning 0, which
>> means success.
> 
> Okay, makes sense.
> 
> In THP=y case, __folio_split() also returns -EINVAL for !large folios,
> but it is not very explicit:
> 
> 	if (new_order >= folio_order(folio))
> 		return -EINVAL;
> 
> In THP=y, we also issue warning:
> 
> 	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> 
You mean:

VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);

> Makes sense to do the same here for THP=n. It might help to catch cases
> we do not see with THP=y, like getting non-THP large folios here.
> 

Yeah, I think that is a good idea. Something like this:

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 48c4f91c5b13..4ddf9e87db91 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -588,21 +588,29 @@ static inline int
 split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
                unsigned int new_order)
 {
+       struct folio *folio = page_folio(page);
+
+       VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
        return -EINVAL;
 }
 static inline int split_huge_page(struct page *page)
 {
+       struct folio *folio = page_folio(page);
+
+       VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
        return -EINVAL;
 }

 static inline int split_folio_to_list(struct folio *folio, struct list_head *list)
 {
+       VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
        return -EINVAL;
 }

 static inline int try_folio_split(struct folio *folio, struct page *page,
                struct list_head *list)
 {
+       VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
        return -EINVAL;
 }

--
Pankaj

On Tue, Sep 02, 2025 at 03:02:23PM +0200, Pankaj Raghav wrote:
> >> I was hitting a weird stale content read error and finally ended up with this fix.
> >>
> >> I thought this is a self-contained patch that can already be upstream. My argument is not that this
> >> should not be reachable, but returning -EINVAL will do the right thing instead of returning 0, which
> >> means success.
> > 
> > Okay, makes sense.
> > 
> > In THP=y case, __folio_split() also returns -EINVAL for !large folios,
> > but it is not very explicit:
> > 
> > 	if (new_order >= folio_order(folio))
> > 		return -EINVAL;
> > 
> > In THP=y, we also issue warning:
> > 
> > 	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> > 
> You mean:
> 
> VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);

Yeah, copied wrong line.

> > Makes sense to do the same here for THP=n. It might help to catch cases
> > we do not see with THP=y, like getting non-THP large folios here.
> > 
> 
> Yeah, I think that is a good idea. Something like this:
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 48c4f91c5b13..4ddf9e87db91 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -588,21 +588,29 @@ static inline int
>  split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
>                 unsigned int new_order)
>  {
> +       struct folio *folio = page_folio(page);
> +
> +       VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);

No. Make it unconditional. The point is we don't expect to see any
splitable folios, so no reason to get here at all.

You can try to use BUILD_BUG(), but it can be too messy.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

> > 
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 48c4f91c5b13..4ddf9e87db91 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -588,21 +588,29 @@ static inline int
> >  split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
> >                 unsigned int new_order)
> >  {
> > +       struct folio *folio = page_folio(page);
> > +
> > +       VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
> 
> No. Make it unconditional. The point is we don't expect to see any
> splitable folios, so no reason to get here at all.
> 

Got it.

Just one question though, in a future world where we remove the
dependency between large folios and THP, then we can revert back
this change to do a conditional WARN_ON?

--
Pankaj

On 02.09.25 15:40, Pankaj Raghav (Samsung) wrote:
>>>
>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>> index 48c4f91c5b13..4ddf9e87db91 100644
>>> --- a/include/linux/huge_mm.h
>>> +++ b/include/linux/huge_mm.h
>>> @@ -588,21 +588,29 @@ static inline int
>>>   split_huge_page_to_list_to_order(struct page *page, struct list_head *list,
>>>                  unsigned int new_order)
>>>   {
>>> +       struct folio *folio = page_folio(page);
>>> +
>>> +       VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
>>
>> No. Make it unconditional. The point is we don't expect to see any
>> splitable folios, so no reason to get here at all.
>>
> 
> Got it.
> 
> Just one question though, in a future world where we remove the
> dependency between large folios and THP, then we can revert back
> this change to do a conditional WARN_ON?
I think we would never expect to get called to split something that is 
small. Calling code should be fixed.

-- 
Cheers

David / dhildenb

On 02.09.25 10:40, Pankaj Raghav (Samsung) wrote:
> From: Pankaj Raghav <p.raghav@samsung.com>
> 
> split_huge_page_to_list_[to_order](), split_huge_page() and
> try_folio_split() return 0 on success and error codes on failure.
> 
> When THP is disabled, these functions return 0 indicating success even
> though an error code should be returned as it is not possible to split a
> folio when THP is disabled.
> 
> Make all these functions return -EINVAL to indicate failure instead of
> 0.
> 
> This issue was discovered while experimenting enabling large folios
> without THP and found that returning 0 in these functions is resulting in
> undefined behavior in truncate operations. This change fixes the issue.

Currently large folios that could be split are impossible without THP, 
so why should this be a fix?

-- 
Cheers

David / dhildenb

On 9/2/25 10:43, David Hildenbrand wrote:
> On 02.09.25 10:40, Pankaj Raghav (Samsung) wrote:
>> From: Pankaj Raghav <p.raghav@samsung.com>
>>
>> split_huge_page_to_list_[to_order](), split_huge_page() and
>> try_folio_split() return 0 on success and error codes on failure.
>>
>> When THP is disabled, these functions return 0 indicating success even
>> though an error code should be returned as it is not possible to split a
>> folio when THP is disabled.
>>
>> Make all these functions return -EINVAL to indicate failure instead of
>> 0.
>>
>> This issue was discovered while experimenting enabling large folios
>> without THP and found that returning 0 in these functions is resulting in
>> undefined behavior in truncate operations. This change fixes the issue.
> 
> Currently large folios that could be split are impossible without THP, so why should this be a fix?
> 
I was debating with myself whether it will classify as a fix or not. I have had people tell me that
I should mention it as fix even though I didn't feel like it ;)

But I agree with you, we can't hit this issue in the upstream kernel. I can remove the fixes tag in
the next version.

--
Pankaj

On 02.09.25 10:59, Pankaj Raghav wrote:
> 
> On 9/2/25 10:43, David Hildenbrand wrote:
>> On 02.09.25 10:40, Pankaj Raghav (Samsung) wrote:
>>> From: Pankaj Raghav <p.raghav@samsung.com>
>>>
>>> split_huge_page_to_list_[to_order](), split_huge_page() and
>>> try_folio_split() return 0 on success and error codes on failure.
>>>
>>> When THP is disabled, these functions return 0 indicating success even
>>> though an error code should be returned as it is not possible to split a
>>> folio when THP is disabled.
>>>
>>> Make all these functions return -EINVAL to indicate failure instead of
>>> 0.
>>>
>>> This issue was discovered while experimenting enabling large folios
>>> without THP and found that returning 0 in these functions is resulting in
>>> undefined behavior in truncate operations. This change fixes the issue.
>>
>> Currently large folios that could be split are impossible without THP, so why should this be a fix?
>>
> I was debating with myself whether it will classify as a fix or not. I have had people tell me that
> I should mention it as fix even though I didn't feel like it ;)
> 
> But I agree with you, we can't hit this issue in the upstream kernel. I can remove the fixes tag in
> the next version.

No need to resend, I think Andrew can just drop the comment.

I'll not that I assume with any large folio support (except unsplittable 
things like hugetlb), we would require splitting support.

So calling these functions would rather indicate a bug I think.

Fine with making them return -EINVAL.

With the Fixes: tag dropped

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb