[PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[Gavin Shan]

Found by code inspection. There are two places where the parameter
passed to page_mapcount_is_type() is (page->__mapcount), which is
correct since it should be one more than the value, as explained in
the comments to page_mapcount_is_type(): (a) page_has_type() in
page-flags.h (b) __dump_folio() in mm/debug.c

PATCH[1] fixes the parameter for (a)
PATCH[2] fixes the parameter for (b)

Gavin Shan (2):
  mm: Fix parameter passed to page_mapcount_is_type()
  mm/debug: Fix parameter passed to page_mapcount_is_type()

 include/linux/page-flags.h | 2 +-
 mm/debug.c                 | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.48.1

[PATCH 1/2] mm: Fix parameter passed to page_mapcount_is_type()

[Gavin Shan]

As the comments of page_mapcount_is_type() indicate, the parameter
passed to the function should be one more than page->__mapcount.
However, page->__mapcount (equivalent to page->page_type) is passed to
the function by commit 4ffca5a96678 ("mm: support only one page_type per
page") where page_type_has_type() is replaced by page_mapcount_is_type(),
but the parameter isn't adjusted.

Fix the parameter passed to page_mapcount_is_type() to be (page->__mapcount
+ 1).

Fixes: 4ffca5a96678 ("mm: support only one page_type per page")
Cc: stable@vger.kernel.org # v6.12+
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 include/linux/page-flags.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 36d283552f80..ad87b4cf1f9a 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -950,7 +950,7 @@ static inline bool page_mapcount_is_type(unsigned int mapcount)
 
 static inline bool page_has_type(const struct page *page)
 {
-	return page_mapcount_is_type(data_race(page->page_type));
+	return page_mapcount_is_type(data_race(page->page_type) + 1);
 }
 
 #define FOLIO_TYPE_OPS(lname, fname)					\
-- 
2.48.1

[PATCH 2/2] mm/debug: Fix parameter passed to page_mapcount_is_type()

[Gavin Shan]

As the comments of page_mapcount_is_type() indicate, the parameter passed
to the function should be one more than page->__mapcount. However,
page->__mapcount is passed to the function by commit 4ffca5a96678
("mm: support only one page_type per page") where page_type_has_type()
is replaced by page_mapcount_is_type(), but the parameter isn't adjusted.

Fix the parameter passed to page_mapcount_is_type() to be (page->__mapcount
+ 1).

Fixes: 4ffca5a96678 ("mm: support only one page_type per page")
Cc: stable@vger.kernel.org # v6.12+
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 mm/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/debug.c b/mm/debug.c
index 8d2acf432385..b6bd9555ec7b 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -71,10 +71,10 @@ static void __dump_folio(struct folio *folio, struct page *page,
 		unsigned long pfn, unsigned long idx)
 {
 	struct address_space *mapping = folio_mapping(folio);
-	int mapcount = atomic_read(&page->_mapcount);
+	int mapcount = atomic_read(&page->_mapcount) + 1;
 	char *type = "";
 
-	mapcount = page_mapcount_is_type(mapcount) ? 0 : mapcount + 1;
+	mapcount = page_mapcount_is_type(mapcount) ? 0 : mapcount;
 	pr_warn("page: refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
 			folio_ref_count(folio), mapcount, mapping,
 			folio->index + idx, pfn);
-- 
2.48.1

Re: [PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[Gavin Shan]

On 3/21/25 3:31 PM, Gavin Shan wrote:
> Found by code inspection. There are two places where the parameter
> passed to page_mapcount_is_type() is (page->__mapcount), which is
> correct since it should be one more than the value, as explained in
   ^^^^^^^
   s/correct/incorrect

> the comments to page_mapcount_is_type(): (a) page_has_type() in
> page-flags.h (b) __dump_folio() in mm/debug.c
> 
> PATCH[1] fixes the parameter for (a)
> PATCH[2] fixes the parameter for (b)
> 
> Gavin Shan (2):
>    mm: Fix parameter passed to page_mapcount_is_type()
>    mm/debug: Fix parameter passed to page_mapcount_is_type()
> 
>   include/linux/page-flags.h | 2 +-
>   mm/debug.c                 | 4 ++--
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 

Re: [PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[Vlastimil Babka]

On 3/21/25 06:31, Gavin Shan wrote:
> Found by code inspection. There are two places where the parameter
> passed to page_mapcount_is_type() is (page->__mapcount), which is
> correct since it should be one more than the value, as explained in
> the comments to page_mapcount_is_type(): (a) page_has_type() in
> page-flags.h (b) __dump_folio() in mm/debug.c

IIUC you are right. Luckily thanks to the the PGTY_mapcount_underflow limit,
this off-by-one error doesn't currently cause visible issues i.e.
misclassifications legitimate mapcount as page type and vice versa, right?
We'd have to have a mapcount underflown severely right to the limit to make
that off-by-one error cross it?

I wonder if a more future-proof solution would be to redefine
page_mapcount_is_type() instead to not subtract. But I'll leave that to willy.

> PATCH[1] fixes the parameter for (a)
> PATCH[2] fixes the parameter for (b)
> 
> Gavin Shan (2):
>   mm: Fix parameter passed to page_mapcount_is_type()
>   mm/debug: Fix parameter passed to page_mapcount_is_type()
> 
>  include/linux/page-flags.h | 2 +-
>  mm/debug.c                 | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 

Re: [PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[David Hildenbrand]

On 21.03.25 10:23, Vlastimil Babka wrote:
> On 3/21/25 06:31, Gavin Shan wrote:
>> Found by code inspection. There are two places where the parameter
>> passed to page_mapcount_is_type() is (page->__mapcount), which is
>> correct since it should be one more than the value, as explained in
>> the comments to page_mapcount_is_type(): (a) page_has_type() in
>> page-flags.h (b) __dump_folio() in mm/debug.c
> 
> IIUC you are right. Luckily thanks to the the PGTY_mapcount_underflow limit,
> this off-by-one error doesn't currently cause visible issues i.e.
> misclassifications legitimate mapcount as page type and vice versa, right?
> We'd have to have a mapcount underflown severely right to the limit to make
> that off-by-one error cross it?

Agreed. Likely not stable material because it isn't actually fixing 
anything (because of the safety gaps).

> 
> I wonder if a more future-proof solution would be to redefine
> page_mapcount_is_type() instead to not subtract. But I'll leave that to willy.

With upcoming changes around that, likely best to leave that alone. I 
expect page_mapcount_is_type() to completely vanish.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm: Fix parameter passed to page_mapcount_is_type()

[David Hildenbrand]

On 21.03.25 06:31, Gavin Shan wrote:
> As the comments of page_mapcount_is_type() indicate, the parameter
> passed to the function should be one more than page->__mapcount.
> However, page->__mapcount (equivalent to page->page_type) is passed to
> the function by commit 4ffca5a96678 ("mm: support only one page_type per
> page") where page_type_has_type() is replaced by page_mapcount_is_type(),
> but the parameter isn't adjusted.
> 
> Fix the parameter passed to page_mapcount_is_type() to be (page->__mapcount
> + 1).
> 
> Fixes: 4ffca5a96678 ("mm: support only one page_type per page")
> Cc: stable@vger.kernel.org # v6.12+
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   include/linux/page-flags.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> index 36d283552f80..ad87b4cf1f9a 100644
> --- a/include/linux/page-flags.h
> +++ b/include/linux/page-flags.h
> @@ -950,7 +950,7 @@ static inline bool page_mapcount_is_type(unsigned int mapcount)
>   
>   static inline bool page_has_type(const struct page *page)
>   {
> -	return page_mapcount_is_type(data_race(page->page_type));
> +	return page_mapcount_is_type(data_race(page->page_type) + 1);

Probably we should just call page_type_has_type() instead?

-- 
Cheers,

David / dhildenb

Re: [PATCH 2/2] mm/debug: Fix parameter passed to page_mapcount_is_type()

[David Hildenbrand]

On 21.03.25 06:31, Gavin Shan wrote:
> As the comments of page_mapcount_is_type() indicate, the parameter passed
> to the function should be one more than page->__mapcount. However,
> page->__mapcount is passed to the function by commit 4ffca5a96678
> ("mm: support only one page_type per page") where page_type_has_type()
> is replaced by page_mapcount_is_type(), but the parameter isn't adjusted.
> 
> Fix the parameter passed to page_mapcount_is_type() to be (page->__mapcount
> + 1).
> 
> Fixes: 4ffca5a96678 ("mm: support only one page_type per page")
> Cc: stable@vger.kernel.org # v6.12+
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   mm/debug.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index 8d2acf432385..b6bd9555ec7b 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -71,10 +71,10 @@ static void __dump_folio(struct folio *folio, struct page *page,
>   		unsigned long pfn, unsigned long idx)
>   {
>   	struct address_space *mapping = folio_mapping(folio);
> -	int mapcount = atomic_read(&page->_mapcount);
> +	int mapcount = atomic_read(&page->_mapcount) + 1;
>   	char *type = "";
>   
> -	mapcount = page_mapcount_is_type(mapcount) ? 0 : mapcount + 1;
> +	mapcount = page_mapcount_is_type(mapcount) ? 0 : mapcount;
>   	pr_warn("page: refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
>   			folio_ref_count(folio), mapcount, mapping,
>   			folio->index + idx, pfn);

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


-- 
Cheers,

David / dhildenb

Re: [PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[Gavin Shan]

On 3/21/25 8:11 PM, David Hildenbrand wrote:
> On 21.03.25 10:23, Vlastimil Babka wrote:
>> On 3/21/25 06:31, Gavin Shan wrote:
>>> Found by code inspection. There are two places where the parameter
>>> passed to page_mapcount_is_type() is (page->__mapcount), which is
>>> correct since it should be one more than the value, as explained in
>>> the comments to page_mapcount_is_type(): (a) page_has_type() in
>>> page-flags.h (b) __dump_folio() in mm/debug.c
>>
>> IIUC you are right. Luckily thanks to the the PGTY_mapcount_underflow limit,
>> this off-by-one error doesn't currently cause visible issues i.e.
>> misclassifications legitimate mapcount as page type and vice versa, right?
>> We'd have to have a mapcount underflown severely right to the limit to make
>> that off-by-one error cross it?
> 
> Agreed. Likely not stable material because it isn't actually fixing anything (because of the safety gaps).
> 

Yes, it shouldn't cause any visible impacts so far due to the gap.
I just found the issue by code inspection. Lets drop the fix tags
in v2.

>>
>> I wonder if a more future-proof solution would be to redefine
>> page_mapcount_is_type() instead to not subtract. But I'll leave that to willy.
> 
> With upcoming changes around that, likely best to leave that alone. I expect page_mapcount_is_type() to completely vanish.
> 

+1 to remove page_mapcount_is_type(). After Willy confirms, I can post
an extra series to do it if needed.

Thanks,
Gavin

Re: [PATCH 1/2] mm: Fix parameter passed to page_mapcount_is_type()

[Gavin Shan]

On 3/21/25 8:13 PM, David Hildenbrand wrote:
> On 21.03.25 06:31, Gavin Shan wrote:
>> As the comments of page_mapcount_is_type() indicate, the parameter
>> passed to the function should be one more than page->__mapcount.
>> However, page->__mapcount (equivalent to page->page_type) is passed to
>> the function by commit 4ffca5a96678 ("mm: support only one page_type per
>> page") where page_type_has_type() is replaced by page_mapcount_is_type(),
>> but the parameter isn't adjusted.
>>
>> Fix the parameter passed to page_mapcount_is_type() to be (page->__mapcount
>> + 1).
>>
>> Fixes: 4ffca5a96678 ("mm: support only one page_type per page")
>> Cc: stable@vger.kernel.org # v6.12+
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   include/linux/page-flags.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
>> index 36d283552f80..ad87b4cf1f9a 100644
>> --- a/include/linux/page-flags.h
>> +++ b/include/linux/page-flags.h
>> @@ -950,7 +950,7 @@ static inline bool page_mapcount_is_type(unsigned int mapcount)
>>   static inline bool page_has_type(const struct page *page)
>>   {
>> -    return page_mapcount_is_type(data_race(page->page_type));
>> +    return page_mapcount_is_type(data_race(page->page_type) + 1);
> 
> Probably we should just call page_type_has_type() instead?
> 

Yes, page_type_has_type() is better. It will be used in v2.

Thanks,
Gavin

Re: [PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[David Hildenbrand]

On 21.03.25 12:25, Gavin Shan wrote:
> On 3/21/25 8:11 PM, David Hildenbrand wrote:
>> On 21.03.25 10:23, Vlastimil Babka wrote:
>>> On 3/21/25 06:31, Gavin Shan wrote:
>>>> Found by code inspection. There are two places where the parameter
>>>> passed to page_mapcount_is_type() is (page->__mapcount), which is
>>>> correct since it should be one more than the value, as explained in
>>>> the comments to page_mapcount_is_type(): (a) page_has_type() in
>>>> page-flags.h (b) __dump_folio() in mm/debug.c
>>>
>>> IIUC you are right. Luckily thanks to the the PGTY_mapcount_underflow limit,
>>> this off-by-one error doesn't currently cause visible issues i.e.
>>> misclassifications legitimate mapcount as page type and vice versa, right?
>>> We'd have to have a mapcount underflown severely right to the limit to make
>>> that off-by-one error cross it?
>>
>> Agreed. Likely not stable material because it isn't actually fixing anything (because of the safety gaps).
>>
> 
> Yes, it shouldn't cause any visible impacts so far due to the gap.
> I just found the issue by code inspection. Lets drop the fix tags
> in v2.
> 
>>>
>>> I wonder if a more future-proof solution would be to redefine
>>> page_mapcount_is_type() instead to not subtract. But I'll leave that to willy.
>>
>> With upcoming changes around that, likely best to leave that alone. I expect page_mapcount_is_type() to completely vanish.
>>
> 
> +1 to remove page_mapcount_is_type(). After Willy confirms, I can post
> an extra series to do it if needed.

I think we should only do that one Willy splits struct folio off from, 
struct page, storing the type elsewhere. For now, we should likely just 
leave it as is.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] mm: Fix parameter passed to page_mapcount_is_type()

[David Hildenbrand]

On 21.03.25 12:26, Gavin Shan wrote:
> On 3/21/25 8:13 PM, David Hildenbrand wrote:
>> On 21.03.25 06:31, Gavin Shan wrote:
>>> As the comments of page_mapcount_is_type() indicate, the parameter
>>> passed to the function should be one more than page->__mapcount.
>>> However, page->__mapcount (equivalent to page->page_type) is passed to
>>> the function by commit 4ffca5a96678 ("mm: support only one page_type per
>>> page") where page_type_has_type() is replaced by page_mapcount_is_type(),
>>> but the parameter isn't adjusted.
>>>
>>> Fix the parameter passed to page_mapcount_is_type() to be (page->__mapcount
>>> + 1).
>>>
>>> Fixes: 4ffca5a96678 ("mm: support only one page_type per page")
>>> Cc: stable@vger.kernel.org # v6.12+
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>    include/linux/page-flags.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
>>> index 36d283552f80..ad87b4cf1f9a 100644
>>> --- a/include/linux/page-flags.h
>>> +++ b/include/linux/page-flags.h
>>> @@ -950,7 +950,7 @@ static inline bool page_mapcount_is_type(unsigned int mapcount)
>>>    static inline bool page_has_type(const struct page *page)
>>>    {
>>> -    return page_mapcount_is_type(data_race(page->page_type));
>>> +    return page_mapcount_is_type(data_race(page->page_type) + 1);
>>
>> Probably we should just call page_type_has_type() instead?
>>
> 
> Yes, page_type_has_type() is better. It will be used in v2.


Feel free to add my

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

-- 
Cheers,

David / dhildenb

Re: [PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[Vlastimil Babka]

On 3/21/25 12:25, Gavin Shan wrote:
> On 3/21/25 8:11 PM, David Hildenbrand wrote:
>> On 21.03.25 10:23, Vlastimil Babka wrote:
>>> On 3/21/25 06:31, Gavin Shan wrote:
>>>> Found by code inspection. There are two places where the parameter
>>>> passed to page_mapcount_is_type() is (page->__mapcount), which is
>>>> correct since it should be one more than the value, as explained in
>>>> the comments to page_mapcount_is_type(): (a) page_has_type() in
>>>> page-flags.h (b) __dump_folio() in mm/debug.c
>>>
>>> IIUC you are right. Luckily thanks to the the PGTY_mapcount_underflow limit,
>>> this off-by-one error doesn't currently cause visible issues i.e.
>>> misclassifications legitimate mapcount as page type and vice versa, right?
>>> We'd have to have a mapcount underflown severely right to the limit to make
>>> that off-by-one error cross it?
>> 
>> Agreed. Likely not stable material because it isn't actually fixing anything (because of the safety gaps).
>> 
> 
> Yes, it shouldn't cause any visible impacts so far due to the gap.

Thanks for confirming, please state that in the commit log/cover letter too.

> I just found the issue by code inspection. Lets drop the fix tags
> in v2.

Fixes: tag is fine and correct, just Cc: stable is unnecessary.
Thanks.

>>>
>>> I wonder if a more future-proof solution would be to redefine
>>> page_mapcount_is_type() instead to not subtract. But I'll leave that to willy.
>> 
>> With upcoming changes around that, likely best to leave that alone. I expect page_mapcount_is_type() to completely vanish.
>> 
> 
> +1 to remove page_mapcount_is_type(). After Willy confirms, I can post
> an extra series to do it if needed.
> 
> Thanks,
> Gavin
> 

Re: [PATCH 0/2] Fix parameter passed to page_mapcount_is_type()

[Gavin Shan]

On 3/21/25 9:33 PM, Vlastimil Babka wrote:
> On 3/21/25 12:25, Gavin Shan wrote:
>> On 3/21/25 8:11 PM, David Hildenbrand wrote:
>>> On 21.03.25 10:23, Vlastimil Babka wrote:
>>>> On 3/21/25 06:31, Gavin Shan wrote:
>>>>> Found by code inspection. There are two places where the parameter
>>>>> passed to page_mapcount_is_type() is (page->__mapcount), which is
>>>>> correct since it should be one more than the value, as explained in
>>>>> the comments to page_mapcount_is_type(): (a) page_has_type() in
>>>>> page-flags.h (b) __dump_folio() in mm/debug.c
>>>>
>>>> IIUC you are right. Luckily thanks to the the PGTY_mapcount_underflow limit,
>>>> this off-by-one error doesn't currently cause visible issues i.e.
>>>> misclassifications legitimate mapcount as page type and vice versa, right?
>>>> We'd have to have a mapcount underflown severely right to the limit to make
>>>> that off-by-one error cross it?
>>>
>>> Agreed. Likely not stable material because it isn't actually fixing anything (because of the safety gaps).
>>>
>>
>> Yes, it shouldn't cause any visible impacts so far due to the gap.
> 
> Thanks for confirming, please state that in the commit log/cover letter too.
> 

Yes, the commit log and cover letter has been improved for this in v2.

>> I just found the issue by code inspection. Lets drop the fix tags
>> in v2.
> 
> Fixes: tag is fine and correct, just Cc: stable is unnecessary.
> 

Thanks for the hints. The 'Cc: stable' tag has been dropped, but the
'Fixes:' tag is kept in v2, which was posted.

https://lore.kernel.org/linux-mm/20250321120222.1456770-1-gshan@redhat.com/T/#t

>>>>
>>>> I wonder if a more future-proof solution would be to redefine
>>>> page_mapcount_is_type() instead to not subtract. But I'll leave that to willy.
>>>
>>> With upcoming changes around that, likely best to leave that alone. I expect page_mapcount_is_type() to completely vanish.
>>>
>>
>> +1 to remove page_mapcount_is_type(). After Willy confirms, I can post
>> an extra series to do it if needed.
>>

Thanks,
Gavin