linux-kernel - Re: [PATCH 2/2] sched: Consider task_struct::saved_state in wait_task

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]

Date:   Mon, 25 Jul 2022 18:47:58 +0100
From:   Valentin Schneider <vschneid@...hat.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Ben Segall <bsegall@...gle.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Mel Gorman <mgorman@...e.de>, Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [PATCH 2/2] sched: Consider task_struct::saved_state in
 wait_task_inactive().

On 20/07/22 17:44, Sebastian Andrzej Siewior wrote:
> Ptrace is using wait_task_inactive() to wait for the tracee to reach a
> certain task state. On PREEMPT_RT that state may be stored in
> task_struct::saved_state while the tracee blocks on a sleeping lock and
> task_struct::__state is set to TASK_RTLOCK_WAIT.
> It is not possible to check only for TASK_RTLOCK_WAIT to be sure that the task
> is blocked on a sleeping lock because during wake up (after the sleeping lock
> has been acquired) the task state is set TASK_RUNNING. After the task in on CPU
> and acquired the pi_lock it will reset the state accordingly but until then
> TASK_RUNNING will be observed (with the desired state saved in saved_state).
>
> Check also for task_struct::saved_state if the desired match was not found in
> task_struct::__state on PREEMPT_RT. If the state was found in saved_state, wait
> until the task is idle and state is visible in task_struct::__state.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>

I have a suggestion and a comment below, but other than that this looks OK.

Reviewed-by: Valentin Schneider <vschneid@...hat.com>

> ---
>  kernel/sched/core.c |   46 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3257,6 +3257,40 @@ int migrate_swap(struct task_struct *cur
>  }
>  #endif /* CONFIG_NUMA_BALANCING */
>
> +#ifdef CONFIG_PREEMPT_RT

Would something like the below be useful?

/*
 * If p->saved_state is anything else than TASK_RUNNING, then p blocked on an
 * rtlock *before* voluntarily calling into schedule() after setting its state
 * to X. For things like ptrace (X=TASK_TRACED), the task could have more work
 * to do upon acquiring the lock before whoever called wait_task_inactive()
 * should return. IOW, we have to wait for:
 *
 *   p.saved_state = TASK_RUNNING
 *   p.__state     = X
 *
 * which implies the task isn't blocked on an RT lock and got to schedule() by
 * itself.
 *
 * Also see comments in ttwu_state_match().
 */

> +static __always_inline bool state_mismatch(struct task_struct *p, unsigned int match_state)
> +{
> +	unsigned long flags;
> +	bool mismatch;
> +
> +	raw_spin_lock_irqsave(&p->pi_lock, flags);
> +	mismatch = READ_ONCE(p->__state) != match_state &&
> +		READ_ONCE(p->saved_state) != match_state;
> +	raw_spin_unlock_irqrestore(&p->pi_lock, flags);
> +	return mismatch;
> +}
> +static __always_inline bool state_match(struct task_struct *p, unsigned int match_state,
> +					bool *wait)
> +{
> +	if (READ_ONCE(p->__state) == match_state)
> +		return true;
> +	if (READ_ONCE(p->saved_state) != match_state)
> +		return false;
> +	*wait = true;
> +	return true;
> +}
> +#else
> +static __always_inline bool state_mismatch(struct task_struct *p, unsigned int match_state)
> +{
> +	return READ_ONCE(p->__state) != match_state;
> +}
> +static __always_inline bool state_match(struct task_struct *p, unsigned int match_state,
> +					bool *wait)
> +{
> +	return READ_ONCE(p->__state) == match_state;
> +}
> +#endif
> +
>  /*
>   * wait_task_inactive - wait for a thread to unschedule.
>   *
> @@ -3346,7 +3382,7 @@ unsigned long wait_task_inactive(struct
>                * running right now), it's preempted, and we should
>                * yield - it could be a while.
>                */
> -		if (unlikely(queued)) {
> +		if (unlikely(wait)) {

We could be repeatedly doing this for as long as the task is blocked on the
rtlock, but IIUC that's the same story on !PREEMPT_RT if it's just a queued
task preempted by a higher prio task, it may take a while for it to
schedule() and dequeue...

>                       ktime_t to = NSEC_PER_SEC / HZ;
>
>                       set_current_state(TASK_UNINTERRUPTIBLE);