Skip to contentSkip to conversation

mm/damon/sysfs-schemes: fix double increment of nr_regions

RFC PATCH In review Vineet Agarwal 3 participants 5 messages May 11 – May 13

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 mm/damon/sysfs-schemes: fix double increment of nr_regions

damos_sysfs_populate_region_dir() increments sysfs_regions->nr_regions twice when adding a new region:
once explicitly before kobject_init_and_add(), and once again through the post-increment used for the kobject name.

As a result, nr_regions no longer matches the actual number of live regions, and region directory names skip numbers (1, 3, 5, ...).

Use the already incremented value for naming instead of incrementing nr_regions a second time.

---
 mm/damon/sysfs-schemes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/damon/sysfs-schemes.c +1 -1
index 622c3799db87..5d966ac86419 100644
@@ -2998,7 +2998,7 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
 	if (kobject_init_and_add(&region->kobj,
 				&damon_sysfs_scheme_region_ktype,
 				&sysfs_regions->kobj, "%d",
-				sysfs_regions->nr_regions++)) {
+				sysfs_regions->nr_regions)) {
 		kobject_put(&region->kobj);
 	}
 }
Signature · 1 line
2.54.0
maintainer
#1 lore raw
Quoted 4 lines · On Tue, 12 May 2026 00:42:15 +0530 Vineet Agarwal…

damos_sysfs_populate_region_dir() increments sysfs_regions->nr_regions twice when adding a new region:
once explicitly before kobject_init_and_add(), and once again through the post-increment used for the kobject name.

Nice catch!

Quoted 3 lines

As a result, nr_regions no longer matches the actual number of live regions,

Nonetheless, this is not causing real issues. The the nr_regions is not exposed to the user space. Similar data structures in the file use arrays for managing sub-directories and use the number field as the size of the array. If that's the case for these region directory data, the code might allocating more-than-needed memory or having some problems. But luckily region directories are managed using linked list. I find no real problems here.

Quoted 2 lines

and region directory names skip
numbers (1, 3, 5, ...).

This is also not causing any real issue. This could arguably make users confused. But still user space could work with this in a reasonable way, by sorting the regions based on the index and/or the address of regions. Actually DAMON user-space tool is handling this. That's why I didn't realize this issue by myself. We recommend human beings to use tools rather than directly interacting with the sysfs interface.

The admin usage document is also not giving wrong assumptions about the names. It only says the names will be integers starting from 0. The real names start from 1, but arguably that doesn't break something, becuase the document doesn't say there will be directory of name '0'...?

Quoted 3 lines

Use the already incremented value for naming instead of incrementing nr_regions a second time.

So, there is no real problems. Please let me know if I'm missing something.

But it is also super clear that this is not an intended behavior but a bug. That needs to be fixed unless it causes unnecessary overhead. Meanwhile, this fix is simple and nice.

So, nice finding and fix! Thank you for sharing, Vineet!

Quoted 2 lines

Seems the current behavior has started since commit 66178e4ec30a ("mm/damon/sysfs: use damos_walk() for update_schemes_tried_{bytes,regions}"). I think it might deserve 'Fixes:' tag if someone claims, but Cc: stable@ may not really needed because this is not causing any real user issue, as I mentioned above.

So, this looks good to me. Vineet, is there anything that you want to sort out before dropping the RFC tag? If not, please feel free to post this again without the RFC tag.

Thanks,
SJ

[...]

#2 lore raw

Hi SJ,

Thanks for the detailed review and explanation.

I also couldn't find any path where nr_regions affects allocation sizing or other important behavior, so this does seem limited to the incorrect count and skipped numbering.

Nothing else to sort out on my end — I've resent the patch without the RFC tag and with a Fixes tag for the commit that introduced the behavior.

Thanks again for taking a look.

Vineet

Quoted 80 lines · SeongJae Park
Quoted 4 lines · On Tue, 12 May 2026 00:42:15 +0530 Vineet Agarwal <…

damos_sysfs_populate_region_dir() increments sysfs_regions->nr_regions twice when adding a new region:
once explicitly before kobject_init_and_add(), and once again through the post-increment used for the kobject name.

Nice catch!

Quoted 3 lines

As a result, nr_regions no longer matches the actual number of live regions,

Nonetheless, this is not causing real issues. The the nr_regions is not exposed to the user space. Similar data structures in the file use arrays for
managing sub-directories and use the number field as the size of the array. If
that's the case for these region directory data, the code might allocating more-than-needed memory or having some problems. But luckily region directories are managed using linked list. I find no real problems here.

Quoted 2 lines

and region directory names skip
numbers (1, 3, 5, ...).

This is also not causing any real issue. This could arguably make users confused. But still user space could work with this in a reasonable way, by
sorting the regions based on the index and/or the address of regions. Actually
DAMON user-space tool is handling this. That's why I didn't realize this issue
by myself. We recommend human beings to use tools rather than directly interacting with the sysfs interface.

The admin usage document is also not giving wrong assumptions about the names.
It only says the names will be integers starting from 0. The real names start
from 1, but arguably that doesn't break something, becuase the document doesn't
say there will be directory of name '0'...?

Quoted 3 lines

Use the already incremented value for naming instead of incrementing nr_regions a second time.

So, there is no real problems. Please let me know if I'm missing something.

But it is also super clear that this is not an intended behavior but a bug. That needs to be fixed unless it causes unnecessary overhead. Meanwhile, this
fix is simple and nice.

So, nice finding and fix! Thank you for sharing, Vineet!

Quoted 2 lines

Seems the current behavior has started since commit 66178e4ec30a ("mm/damon/sysfs: use damos_walk() for update_schemes_tried_{bytes,regions}").
I think it might deserve 'Fixes:' tag if someone claims, but Cc: stable@ may
not really needed because this is not causing any real user issue, as I mentioned above.

So, this looks good to me. Vineet, is there anything that you want to sort out
before dropping the RFC tag? If not, please feel free to post this again without the RFC tag.

Thanks,
SJ

[...]

#3 lore raw
In reply to Vineet Agarwal
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] The patch fails to restore the documented 0-indexed ABI, leaving region directory names…

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The patch fails to restore the documented 0-indexed ABI, leaving region directory names 1-indexed.
- [Critical] Pre-existing: Use-After-Free and list corruption due to synchronous kobject_put() assumption.
- [Medium] Unbalanced nr_regions increment on kobject_init_and_add() failure causes a permanent state leak. --

commit 66d182db7d03e02c982ddd51e0ba311b2642559b
Author: Vineet Agarwal <[email protected]>

mm/damon/sysfs-schemes: fix double increment of nr_regions

This commit changes damos_sysfs_populate_region_dir() to use the already incremented value of nr_regions for naming new region directories, preventing the skipping of directory numbers.

mm/damon/sysfs-schemes.c void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes, · line 3001

Quoted 10 lines
mm/damon/sysfs-schemes.c +1 -1
index 622c3799db87b..5d966ac864193 100644
@@ -2998,7 +2998,7 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
 	if (kobject_init_and_add(&region->kobj,
 				&damon_sysfs_scheme_region_ktype,
 				&sysfs_regions->kobj, "%d",
-				sysfs_regions->nr_regions++)) {
+				sysfs_regions->nr_regions)) {

Does this change the ABI from 0-indexed to 1-indexed? Since nr_regions is incremented just before kobject_init_and_add(), the first directory will be formatted with the value "1". The kernel documentation indicates these directories should start from 0.

mm/damon/sysfs-schemes.c void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes, · line 3002

Quoted 1 line
mm/damon/sysfs-schemes.c +0 -0
 		kobject_put(&region->kobj);

If kobject_init_and_add() fails here, we call kobject_put(&region->kobj) but do not decrement sysfs_regions->nr_regions. Will this permanently inflate the counter and cause subsequent directory names to skip numbers, since damon_sysfs_scheme_regions_rm_dirs() decrements the counter based on the remaining successfully added elements in the list?

There might also be a pre-existing issue with the region list lifecycle. If damon_sysfs_scheme_regions_rm_dirs() iterates regions_list and calls kobject_put() on each region's kobj, does it assume the kobject release function will synchronously remove the region from the list?

If the release is delayed, for instance by external sysfs references or CONFIG_DEBUG_KOBJECT_RELEASE, the object could remain on regions_list. If damos_sysfs_populate_region_dir() then concurrently adds to regions_list with list_add_tail(), could the delayed release race and cause list corruption?

Could a subsequent call to damon_sysfs_scheme_regions_rm_dirs() traverse freed list nodes and call kobject_put() on zero-refcount objects if the release hasn't completed?

Quoted 2 lines
mm/damon/sysfs-schemes.c +0 -0
 	}
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
maintainer
#4 lore raw
Quoted 2 lines · On Wed, 13 May 2026 00:08:38 +0000 [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The patch fails to restore the documented 0-indexed ABI, leaving region directory names 1-indexed.

Not this patch's fault. No real issue.

Quoted 1 line

- [Critical] Pre-existing: Use-After-Free and list corruption due to synchronous kobject_put() assumption.

I will work on this.

Quoted 1 line

- [Medium] Unbalanced nr_regions increment on kobject_init_and_add() failure causes a permanent state leak.

Not this patch's fault. No real issue.

Quoted 11 lines

--

commit 66d182db7d03e02c982ddd51e0ba311b2642559b
Author: Vineet Agarwal <[email protected]>

mm/damon/sysfs-schemes: fix double increment of nr_regions

This commit changes damos_sysfs_populate_region_dir() to use the already incremented value of nr_regions for naming new region directories, preventing the skipping of directory numbers.

Quoted 15 lines

mm/damon/sysfs-schemes.c void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes, · line 3001

Quoted 10 lines
mm/damon/sysfs-schemes.c +1 -1
index 622c3799db87b..5d966ac864193 100644
@@ -2998,7 +2998,7 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
 	if (kobject_init_and_add(&region->kobj,
 				&damon_sysfs_scheme_region_ktype,
 				&sysfs_regions->kobj, "%d",
-				sysfs_regions->nr_regions++)) {
+				sysfs_regions->nr_regions)) {

Does this change the ABI from 0-indexed to 1-indexed? Since nr_regions is incremented just before kobject_init_and_add(), the first directory will be formatted with the value "1". The kernel documentation indicates these directories should start from 0.

Not a new behavior. And it should be fine. Users could reasonably deal with it. Refer to my first reply to the patch for more details.

Quoted 8 lines

mm/damon/sysfs-schemes.c void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes, · line 3002

Quoted 2 lines
mm/damon/sysfs-schemes.c +0 -0
 		kobject_put(&region->kobj);

If kobject_init_and_add() fails here, we call kobject_put(&region->kobj) but do not decrement sysfs_regions->nr_regions. Will this permanently inflate the counter and cause subsequent directory names to skip numbers, since damon_sysfs_scheme_regions_rm_dirs() decrements the counter based on the remaining successfully added elements in the list?

Yes, and it is an old behavior, so nothing to blame for this patch. Also same to the 1-index, users should be able to reasonably handle it.

Quoted 12 lines

There might also be a pre-existing issue with the region list lifecycle. If damon_sysfs_scheme_regions_rm_dirs() iterates regions_list and calls kobject_put() on each region's kobj, does it assume the kobject release function will synchronously remove the region from the list?

If the release is delayed, for instance by external sysfs references or CONFIG_DEBUG_KOBJECT_RELEASE, the object could remain on regions_list. If damos_sysfs_populate_region_dir() then concurrently adds to regions_list with list_add_tail(), could the delayed release race and cause list corruption?

Quoted 3 lines

Could a subsequent call to damon_sysfs_scheme_regions_rm_dirs() traverse freed list nodes and call kobject_put() on zero-refcount objects if the release hasn't completed?

Good finding, I will work on this.

Thanks,
SJ

[...]