memcg: damon: get memcg reference before access
Subsystems (3)
PATCH 1/1 memcg: damon: get memcg reference before access
The commit b74a120bcf507 ("mm/damon/core: implement DAMOS_QUOTA_NODE_MEMCG_USED_BP") added accesses to memcg structure without getting reference to it. This is unsafe. Let's get the reference before accessing the memcg.
--- mm/damon/core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
mm/damon/core.c +6 -2
@@ -2051,13 +2051,15 @@ static unsigned long damos_get_node_memcg_used_bp(rcu_read_lock (); memcg =mem_cgroup_from_id (goal->memcg_id); -rcu_read_unlock (); -if (!memcg) { +if (!memcg || !mem_cgroup_tryget (memcg)) { +rcu_read_unlock ();if (goal->metric ==DAMOS_QUOTA_NODE_MEMCG_USED_BP )return 0 ;else /* DAMOS_QUOTA_NODE_MEMCG_FREE_BP */ return 10000 ; } +rcu_read_unlock (); +mem_cgroup_flush_stats (memcg); lruvec =mem_cgroup_lruvec (memcg,NODE_DATA (goal->nid)); used_pages =lruvec_page_state (lruvec,NR_ACTIVE_ANON );
@@ -2065,6 +2067,8 @@ static unsigned long damos_get_node_memcg_used_bp( used_pages +=lruvec_page_state (lruvec,NR_ACTIVE_FILE ); used_pages +=lruvec_page_state (lruvec,NR_INACTIVE_FILE ); +mem_cgroup_put (memcg); +si_meminfo_node (&i, goal->nid);if (goal->metric ==DAMOS_QUOTA_NODE_MEMCG_USED_BP ) numerator = used_pages;
Signature · 1 line
2.47.3
Quoted 4 lines · On Tue, 23 Dec 2025 19:45:27 -0800 Shakeel Butt <[email protected]> wrote:
The commit
b74a120bcf507("mm/damon/core: implementDAMOS_QUOTA_NODE_MEMCG_USED_BP") added accesses to memcg structure without getting reference to it. This is unsafe. Let's get the reference before accessing the memcg.
Thank you for catching and fixing this!
Nit. On the subject, could we use 'mm/damon/core:' prefix instead of 'memcg:
damon:' for keeping the file's commit log subjects consistent?
Quoted 2 lines
Fixes b74a120bcf507 ("mm/damon/core: implement DAMOS_QUOTA_NODE_MEMCG_USED_BP")
I was firsty thinking we might need to Cc: stable@. But I realized the broken commit has merged into 6.19-rc1. So Cc: stable@ is not needed.
Quoted 1 line
Signed-off-by Shakeel Butt <[email protected]>
Other than the tirivial subject prefix inconsistency, looks good to me.
Quoted 9 lines
--- mm/damon/core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)mm/damon/core.c +0
-0
mm/damon/core.c static unsigned long damos_get_node_memcg_used_bp( · line 2054
Quoted 6 lines
mm/damon/core.c +1
-2@@ -2051,13 +2051,15 @@ static unsigned long damos_get_node_memcg_used_bp(rcu_read_lock (); memcg =mem_cgroup_from_id (goal->memcg_id); -rcu_read_unlock (); -if (!memcg) { +if (!memcg || !mem_cgroup_tryget (memcg)) {
For this part, I was thinking '!memcg' part seems not technically needed because mem_cgroup_tryget() does the check. But I think that's just trivial, so this also looks good to me.
Quoted 22 lines
mm/damon/core.c +5
-0+rcu_read_unlock ();if (goal->metric ==DAMOS_QUOTA_NODE_MEMCG_USED_BP )return 0 ;else /* DAMOS_QUOTA_NODE_MEMCG_FREE_BP */ return 10000 ; } +rcu_read_unlock (); +mem_cgroup_flush_stats (memcg); lruvec =mem_cgroup_lruvec (memcg,NODE_DATA (goal->nid)); used_pages =lruvec_page_state (lruvec,NR_ACTIVE_ANON );@@ -2065,6 +2067,8 @@ static unsigned long damos_get_node_memcg_used_bp( used_pages +=lruvec_page_state (lruvec,NR_ACTIVE_FILE ); used_pages +=lruvec_page_state (lruvec,NR_INACTIVE_FILE ); +mem_cgroup_put (memcg); +si_meminfo_node (&i, goal->nid);if (goal->metric ==DAMOS_QUOTA_NODE_MEMCG_USED_BP ) numerator = used_pages;Signature · 1 line
2.47.3
Thanks,
SJ
[...]
Quoted 11 lines · SeongJae Park
Quoted 4 lines · On Tue, 23 Dec 2025 19:45:27 -0800 Shakeel Butt <[email protected]> wrote:
The commit
b74a120bcf507("mm/damon/core: implementDAMOS_QUOTA_NODE_MEMCG_USED_BP") added accesses to memcg structure without getting reference to it. This is unsafe. Let's get the reference before accessing the memcg.Thank you for catching and fixing this!
Nit. On the subject, could we use '
mm/damon/core:' prefix instead of 'memcg:
damon:' for keeping the file's commit log subjects consistent?
Done.
Quoted 4 lines
Quoted 3 lines
Fixes b74a120bcf507 ("mm/damon/core: implement DAMOS_QUOTA_NODE_MEMCG_USED_BP")
Quoted 9 lines
I was firsty thinking we might need to Cc: stable@. But I realized the broken commit has merged into 6.19-rc1. So Cc: stable@ is not needed.
Quoted 1 line
Signed-off-by Shakeel Butt <[email protected]>Other than the tirivial subject prefix inconsistency, looks good to me.
Reviewed-by SeongJae Park <[email protected]>
Thanks.
Quoted 10 lines
Quoted 10 lines
--- mm/damon/core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)mm/damon/core.c +0
-0@@ -2051,13 +2051,15 @@ static unsigned long damos_get_node_memcg_used_bp(
Quoted 10 lines
mm/damon/core.c static unsigned long damos_get_node_memcg_used_bp( · line 2054
Quoted 5 lines
mm/damon/core.c +1
-2rcu_read_lock (); memcg =mem_cgroup_from_id (goal->memcg_id); -rcu_read_unlock (); -if (!memcg) { +if (!memcg || !mem_cgroup_tryget (memcg)) {For this part, I was thinking '!memcg' part seems not technically needed because
mem_cgroup_tryget()does the check. But I think that's just trivial, so this also looks good to me.
Hmm !memcg check inside mem_cgroup_tryget() is a weird one. It makes mem_cgroup_tryget() to return true for NULL parameter. We can not use mem_cgroup_tryget() as is alone here.
SeongJae Park
maintainerQuoted 2 lines · On Tue, 23 Dec 2025 23:51:55 -0800 Shakeel Butt <[email protected]> wrote:
Quoted 1 line · SeongJae Park
On Tue, 23 Dec 2025 19:45:27 -0800 Shakeel Butt <[email protected]> wrote:
[...]
Quoted 17 lines
Quoted 6 lines
Quoted 6 lines
mm/damon/core.c +0
-0@@ -2051,13 +2051,15 @@ static unsigned long damos_get_node_memcg_used_bp(Quoted 10 lines
mm/damon/core.c static unsigned long damos_get_node_memcg_used_bp( · line 2054
Quoted 5 lines
mm/damon/core.c +1
-2rcu_read_lock (); memcg =mem_cgroup_from_id (goal->memcg_id); -rcu_read_unlock (); -if (!memcg) { +if (!memcg || !mem_cgroup_tryget (memcg)) {For this part, I was thinking '!memcg' part seems not technically needed because
mem_cgroup_tryget()does the check. But I think that's just trivial, so this also looks good to me.
Quoted 3 lines
Hmm !memcg check inside
mem_cgroup_tryget()is a weird one. It makesmem_cgroup_tryget()to return true for NULL parameter. We can not usemem_cgroup_tryget()as is alone here.
Oh, you're right. I should have read it more carefully. Thank you for enlightening me :)
Thanks,
SJ
[...]