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 for Android: free password hash cracker in your pocket

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

Message-ID: <hs3oag7blyg5kkdu6ikbw7f6hefkdfk2qgqqnpothq7yx4qsts@gv2v4dbpfmv6>
Date: Thu, 25 Jul 2024 11:38:31 +0200
From: Michal Koutný <mkoutny@...e.com>
To: xiujianfeng <xiujianfeng@...wei.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

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?

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

Michal

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)