| Message ID | 20250321053148.1434076-1-gshan@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix parameter passed to page_mapcount_is_type() | expand |
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(-) >
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(-) >
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.
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
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.
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 >
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