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] [day] [month] [year] [list]

Message-ID: <ffdb6f77-89b4-cb54-4333-c5d63ef0a698@huaweicloud.com>
Date: Tue, 30 Jul 2024 11:21:37 +0800
From: Xiu Jianfeng <xiujianfeng@...weicloud.com>
To: Michal Koutný <mkoutny@...e.com>
Cc: cgroups@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 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



On 2024/7/25 17:38, Michal Koutný wrote:
> Hello Jianfeng.
> 
> On Tue, Jul 16, 2024 at 11:27:39AM GMT, xiujianfeng <xiujianfeng@...wei.com> wrote:
>> On 2024/7/3 14:59, xiujianfeng wrote:
> ...
>>>         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?
> 
> Two generic notes:
> - event notifications can be rate limited, so users won't necessarily
>   see every change,
> - upon notification it's better to read the event counter/status anyway
>   to base a response on it.
> 
> But your remark is justified, there is no reason in this case for
> "spurious" event notification. It's an omission from v3 version of the
> patch when there had been also pids.events:max.imposed (that'd trigger
> events from D up to the root, it's only internal PIDCG_FORKFAIL now).
> 
> The upwards traversal loop can be simplified and fixed with only
> PIDCG_MAX exposed. Can you send it as a separate patch please?

Hi Michal,

Thanks for your feedback. and I'm sorry I forgot to reply this thread
after sending the patch.

> 
> (Apologies for late response, somehow I didn't see your e-mails.)
> 
> Michal