linux-kernel - Re: [PATCH -next] cgroup/pids: Avoid spurious event notification

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: <k2cfhjs33ch6dd2v3wzrs77dthcgavhaleinaxgt4oulaztekc@pikhtt5e52tc>
Date: Mon, 29 Jul 2024 18:07:53 +0200
From: Michal Koutný <mkoutny@...e.com>
To: Xiu Jianfeng <xiujianfeng@...weicloud.com>
Cc: tj@...nel.org, lizefan.x@...edance.com, hannes@...xchg.org, 
	cgroups@...r.kernel.org, linux-kernel@...r.kernel.org, xiujianfeng@...wei.com
Subject: Re: [PATCH -next] cgroup/pids: Avoid spurious event notification

Hello.

On Mon, Jul 29, 2024 at 10:58:24AM GMT, Xiu Jianfeng <xiujianfeng@...weicloud.com> wrote:
> To address this issue, only the cgroups from 'pids_over_limit' to root
> will have their PIDCG_MAX counter increased and event notifications
> generated.
> 

For completeness here

Fixes: 385a635cacfe0 ("cgroup/pids: Make event counters hierarchical")

> Signed-off-by: Xiu Jianfeng <xiujianfeng@...wei.com>
> ---
>  kernel/cgroup/pids.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)



> @@ -257,15 +256,11 @@ static void pids_event(struct pids_cgroup *pids_forking,
>  	    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]);
> +	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);
>  	}

When I look at it applied altogther, there's one extra notification
(heritage of forkfail events), it should be fixed with:

--- a/kernel/cgroup/pids.c
+++ b/kernel/cgroup/pids.c
@@ -251,10 +251,11 @@ 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;
+       }
 
        atomic64_inc(&pids_over_limit->events_local[PIDCG_MAX]);
        cgroup_file_notify(&pids_over_limit->events_local_file);

Besides that it makes sense to me.

Thanks,
Michal

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