linux-kernel - Re: [PATCH v5 2/5] cgroup/pids: Make event counters hierarchical

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cb0efc16-6df2-72b7-47ea-ce524d428cc1@huawei.com>
Date: Tue, 16 Jul 2024 11:27:39 +0800
From: xiujianfeng <xiujianfeng@...wei.com>
To: Michal Koutný <mkoutny@...e.com>,
	<cgroups@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>
CC: Tejun Heo <tj@...nel.org>, Zefan Li <lizefan.x@...edance.com>, Johannes
 Weiner <hannes@...xchg.org>, Jonathan Corbet <corbet@....net>, Shuah Khan
	<shuah@...nel.org>, Muhammad Usama Anjum <usama.anjum@...labora.com>
Subject: Re: [PATCH v5 2/5] cgroup/pids: Make event counters hierarchical

Hi,

Friendly ping, more comment as below.

On 2024/7/3 14:59, xiujianfeng wrote:
> 
> 
> On 2024/5/21 17:21, Michal Koutný wrote:
>> The pids.events file should honor the hierarchy, so make the events
>> propagate from their origin up to the root on the unified hierarchy. The
>> legacy behavior remains non-hierarchical.
>>
>> Signed-off-by: Michal Koutný <mkoutny@...e.com>
>> --
> [...]
>> diff --git a/kernel/cgroup/pids.c b/kernel/cgroup/pids.c
>> index a557f5c8300b..c09b744d548c 100644
>> --- a/kernel/cgroup/pids.c
>> +++ b/kernel/cgroup/pids.c
>> @@ -238,6 +238,34 @@ static void pids_cancel_attach(struct cgroup_taskset *tset)
>>  	}
>>  }
>>  
>> +static void pids_event(struct pids_cgroup *pids_forking,
>> +		       struct pids_cgroup *pids_over_limit)
>> +{
>> +	struct pids_cgroup *p = pids_forking;
>> +	bool limit = false;
>> +
>> +	for (; parent_pids(p); p = parent_pids(p)) {
>> +		/* Only log the first time limit is hit. */
>> +		if (atomic64_inc_return(&p->events[PIDCG_FORKFAIL]) == 1) {
>> +			pr_info("cgroup: fork rejected by pids controller in ");
>> +			pr_cont_cgroup_path(p->css.cgroup);
>> +			pr_cont("\n");
>> +		}
>> +		cgroup_file_notify(&p->events_file);
>> +
>> +		if (!cgroup_subsys_on_dfl(pids_cgrp_subsys) ||
>> +		    cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
>> +			break;
>> +
>> +		if (p == pids_over_limit)
>> +			limit = true;
>> +		if (limit)
>> +			atomic64_inc(&p->events[PIDCG_MAX]);
>> +
>> +		cgroup_file_notify(&p->events_file);
> 
> Hi Michal,
> 
> I have doubts about this code. To better illustrate the problem, I am
> posting the final code here.
> 
> static void pids_event(struct pids_cgroup *pids_forking,
>                        struct pids_cgroup *pids_over_limit)
> {
> ...
>         cgroup_file_notify(&p->events_local_file);
>         if (!cgroup_subsys_on_dfl(pids_cgrp_subsys) ||
>             cgrp_dfl_root.flags & CGRP_ROOT_PIDS_LOCAL_EVENTS)
>                 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]);
> 
>                 cgroup_file_notify(&p->events_file);
>         }
> }
> 
> Consider this scenario: there are 4 groups A, B, C,and D. The
> relationships are as follows, the latter is the child of the former:
> 
> root->A->B->C->D
> 
> Then the user is polling on C.pids.events. When a process in D forks and
> fails due to B.max restrictions(pids_forking is D, and pids_over_limit
> is B), the user is awakened. However, when the user reads C.pids.events,
> he will find that the content has not changed. because the 'limit' is
> set to true started from B, and C.pids.events shows as below:
> 
> seq_printf(sf, "max %lld\n", (s64)atomic64_read(&events[PIDCG_MAX]));
> 
> Wouldn't this behavior confuse the user? Should the code to be changed
> to this?
> 
> if (limit) {
>       atomic64_inc(&p->events[PIDCG_MAX]);
>       cgroup_file_notify(&p->events_file);
> }
>

or should the for loop be changed to the following?

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(&pt->events[PIDCG_MAX]);
    cgroup_file_notify(&p->events_file);
}

The current behaviour is quite different from other subsys, such as
memcg, that make me confused, maybe I am missing something.

it's appreciated if anyone could respond.