linux-kernel - Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

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: <xhsmhv7wrb3sc.mognet@vschneid-thinkpadt14sgen2i.remote.csb>
Date: Wed, 13 Nov 2024 15:34:59 +0100
From: Valentin Schneider <vschneid@...hat.com>
To: Chen Ridong <chenridong@...weicloud.com>, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
 dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de
Cc: linux-kernel@...r.kernel.org, chenridong@...wei.com,
 wangweiyang2@...wei.com
Subject: Re: [PATCH] freezer, sched: report the frozen task stat as 'D'

On 11/11/24 13:54, Chen Ridong wrote:
> From: Chen Ridong <chenridong@...wei.com>
>
> Before the commit f5d39b020809 ("freezer,sched: Rewrite core freezer
> logic"), the frozen task stat was reported as 'D' in cgroup v1.
> However, after rewriting core freezer logic, the frozen task stat is
> reported as 'R'. This is confusing, especially when a task with stat of
> 'S' is frozen.
>
> This can be reproduced as bellow step:
> cd /sys/fs/cgroup/freezer/
> mkdir test
> sleep 1000 &
> [1] 739         // task whose stat is 'S'
> echo 739 > test/cgroup.procs
> echo FROZEN > test/freezer.state
> ps -aux | grep 739
> root     739  0.1  0.0   8376  1812 pts/0    R    10:56   0:00 sleep 1000
>
> As shown above, a task whose stat is 'S' was changed to 'R' when it was
> frozen. To solve this issue, simply maintain the same reported state as
> before the rewrite.
>
> Signed-off-by: Chen Ridong <chenridong@...wei.com>
> ---
>  include/linux/sched.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index f0f23efdb245..878acce2f8d6 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1630,8 +1630,9 @@ static inline unsigned int __task_state_index(unsigned int tsk_state,
>  	 * We're lying here, but rather than expose a completely new task state
>  	 * to userspace, we can make this appear as if the task has gone through
>  	 * a regular rt_mutex_lock() call.
> +	 * Report the frozen task uninterruptible.
>  	 */
> -	if (tsk_state & TASK_RTLOCK_WAIT)
> +	if (tsk_state & TASK_RTLOCK_WAIT || tsk_state & TASK_FROZEN)

Hmph, for RTLOCK this was good enough since !PREEMPT_RT really would be
TASK_UNINTERRUPTIBLE. 

For the freezer AFAICT there's more variation, following your example:

  crash> task -R __state,saved_state 195
  PID: 195      TASK: ffff888004abab80  CPU: 1    COMMAND: "sh"
    __state = 32768    -> 0x8000 aka TASK_FROZEN
    saved_state = 8193 -> 0x2001 aka TASK_INTERRUPTIBLE | TASK_FREEZABLE

so we ought to read ->saved_state, but as pointed out in [1] this is
racy...

[1]: https://lore.kernel.org/lkml/20220120162520.570782-3-valentin.schneider@arm.com/

>  		state = TASK_UNINTERRUPTIBLE;
>  
>  	return fls(state);
> -- 
> 2.34.1