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: <f124ce60-196e-2392-c4a9-11cdcacf9927@huawei.com>
Date: Wed, 3 Jul 2024 14:59:36 +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



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);
}