From: Xiu Jianfeng <xiujianfeng@huawei.com>

Currently when a process in a group forks and fails due to it's
parent's max restriction, all the cgroups from 'pids_forking' to root
will generate event notifications but only the cgroups from
'pids_over_limit' to root will increase the counter of PIDCG_MAX.

Consider this scenario: there are 4 groups A, B, C,and D, the
relationships are as follows, and user is watching on C.pids.events.

root->A->B->C->D

When a process in D forks and fails due to B.max restriction, the
user will get a spurious event notification because when he wakes up
and reads C.pids.events, he will find that the content has not changed.

To address this issue, only the cgroups from 'pids_over_limit' to root
will have their PIDCG_MAX counters increased and event notifications
generated.

Fixes: 385a635cacfe ("cgroup/pids: Make event counters hierarchical")
Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
---
v2: add Fixes tag and fix a extra notification
---
 kernel/cgroup/pids.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup/pids.c b/kernel/cgroup/pids.c
index f5cb0ec45b9d..34aa63d7c9c6 100644
--- a/kernel/cgroup/pids.c
+++ b/kernel/cgroup/pids.c
@@ -244,7 +244,6 @@ static void pids_event(struct pids_cgroup *pids_forking,
 		       struct pids_cgroup *pids_over_limit)
 {
 	struct pids_cgroup *p = pids_forking;
-	bool limit = false;
 
 	/* Only log the first time limit is hit. */
 	if (atomic64_inc_return(&p->events_local[PIDCG_FORKFAIL]) == 1) {
@@ -252,20 +251,17 @@ static void pids_event(struct pids_cgroup *pids_forking,
 		pr_cont_cgroup_path(p->css.cgroup);
 		pr_cont("\n");
 	}
-	cgroup_file_notify(&p->events_local_file);
 	if (!cgroup_subsys_on_dfl(pids_cgrp_subsys) ||
-	    cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
+	    cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS) {
+		cgroup_file_notify(&p->events_local_file);
 		return;
+	}
 
-	for (; parent_pids(p); p = parent_pids(p)) {
-		if (p == pids_over_limit) {
-			limit = true;
-			atomic64_inc(&p->events_local[PIDCG_MAX]);
-			cgroup_file_notify(&p->events_local_file);
-		}
-		if (limit)
-			atomic64_inc(&p->events[PIDCG_MAX]);
+	atomic64_inc(&pids_over_limit->events_local[PIDCG_MAX]);
+	cgroup_file_notify(&pids_over_limit->events_local_file);
 
+	for (p = pids_over_limit; parent_pids(p); p = parent_pids(p)) {
+		atomic64_inc(&p->events[PIDCG_MAX]);
 		cgroup_file_notify(&p->events_file);
 	}
 }
-- 
2.34.1

On Tue, Jul 30, 2024 at 03:29:20AM +0000, Xiu Jianfeng wrote:
> From: Xiu Jianfeng <xiujianfeng@huawei.com>
> 
> Currently when a process in a group forks and fails due to it's
> parent's max restriction, all the cgroups from 'pids_forking' to root
> will generate event notifications but only the cgroups from
> 'pids_over_limit' to root will increase the counter of PIDCG_MAX.
> 
> Consider this scenario: there are 4 groups A, B, C,and D, the
> relationships are as follows, and user is watching on C.pids.events.
> 
> root->A->B->C->D
> 
> When a process in D forks and fails due to B.max restriction, the
> user will get a spurious event notification because when he wakes up
> and reads C.pids.events, he will find that the content has not changed.
> 
> To address this issue, only the cgroups from 'pids_over_limit' to root
> will have their PIDCG_MAX counters increased and event notifications
> generated.
> 
> Fixes: 385a635cacfe ("cgroup/pids: Make event counters hierarchical")
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>

Applied to cgroup/for-6.12.

Note that spurious events are explicitly allowed. Anyone watching an events
file should keep track of the reported values to detect actual events.

Thanks.

-- 
tejun

On 2024/7/31 6:14, Tejun Heo wrote:
> On Tue, Jul 30, 2024 at 03:29:20AM +0000, Xiu Jianfeng wrote:
>> From: Xiu Jianfeng <xiujianfeng@huawei.com>
>>
>> Currently when a process in a group forks and fails due to it's
>> parent's max restriction, all the cgroups from 'pids_forking' to root
>> will generate event notifications but only the cgroups from
>> 'pids_over_limit' to root will increase the counter of PIDCG_MAX.
>>
>> Consider this scenario: there are 4 groups A, B, C,and D, the
>> relationships are as follows, and user is watching on C.pids.events.
>>
>> root->A->B->C->D
>>
>> When a process in D forks and fails due to B.max restriction, the
>> user will get a spurious event notification because when he wakes up
>> and reads C.pids.events, he will find that the content has not changed.
>>
>> To address this issue, only the cgroups from 'pids_over_limit' to root
>> will have their PIDCG_MAX counters increased and event notifications
>> generated.
>>
>> Fixes: 385a635cacfe ("cgroup/pids: Make event counters hierarchical")
>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> 
> Applied to cgroup/for-6.12.
> 
> Note that spurious events are explicitly allowed. Anyone watching an events
> file should keep track of the reported values to detect actual events.

Hi Tejun,

Thank you for your clarification. I thought it was not allowed.

Anyway, it's always good to avoid spurious events and disturbances to
users IMO.

> 
> Thanks.
>