linux-kernel - Re: [PATCH v5 21/23] rv: Add rtapp

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: <20250429172055.QICnVQ2s@linutronix.de>
Date: Tue, 29 Apr 2025 19:20:55 +0200
From: Nam Cao <namcao@...utronix.de>
To: Gabriele Monaco <gmonaco@...hat.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	linux-trace-kernel@...r.kernel.org, linux-kernel@...r.kernel.org,
	john.ogness@...utronix.de, Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Boqun Feng <boqun.feng@...il.com>, Waiman Long <longman@...hat.com>
Subject: Re: [PATCH v5 21/23] rv: Add rtapp_sleep monitor

On Tue, Apr 29, 2025 at 06:01:01PM +0200, Gabriele Monaco wrote:
> On Tue, 2025-04-29 at 14:01 +0200, Nam Cao wrote:
> > Add a monitor for checking that real-time tasks do not go to sleep in
> > a
> > manner that may cause undesirable latency.
> > 
> > Also change
> > 	RV depends on TRACING
> > to
> > 	RV select TRACING
> > to avoid the following recursive dependency:
> > 
> >  error: recursive dependency detected!
> > 	symbol TRACING is selected by PREEMPTIRQ_TRACEPOINTS
> > 	symbol PREEMPTIRQ_TRACEPOINTS depends on TRACE_IRQFLAGS
> > 	symbol TRACE_IRQFLAGS is selected by RV_MON_SLEEP
> > 	symbol RV_MON_SLEEP depends on RV
> > 	symbol RV depends on TRACING
> > 
> > Reviewed-by: Gabriele Monaco <gmonaco@...hat.com>
> > Signed-off-by: Nam Cao <namcao@...utronix.de>
> > ---
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: Will Deacon <will@...nel.org>
> > Cc: Boqun Feng <boqun.feng@...il.com>
> > Cc: Waiman Long <longman@...hat.com>
> > ---
> > 
> > [...]
> > 
> > +RULE = always ((RT and SLEEP) imply (RT_FRIENDLY_SLEEP or
> > ALLOWLIST))
> > +
> > +RT_FRIENDLY_SLEEP = (RT_VALID_SLEEP_REASON or KERNEL_THREAD)
> > +                and ((not WAKE) until RT_FRIENDLY_WAKE)
> > +
> > +RT_VALID_SLEEP_REASON = PI_FUTEX
> > +                     or RT_FRIENDLY_NANOSLEEP
> > +
> > +RT_FRIENDLY_NANOSLEEP = CLOCK_NANOSLEEP
> > +                    and NANOSLEEP_TIMER_ABSTIME
> > +                    and NANOSLEEP_CLOCK_MONOTONIC
> > +
> > +RT_FRIENDLY_WAKE = WOKEN_BY_EQUAL_OR_HIGHER_PRIO
> > +                or WOKEN_BY_HARDIRQ
> > +                or WOKEN_BY_NMI
> > +                or KTHREAD_SHOULD_STOP
> > +
> > +ALLOWLIST = BLOCK_ON_RT_MUTEX
> > +         or TASK_IS_RCU
> > +         or TASK_IS_MIGRATION
> 
> So, just thinking out loud, PI_FUTEX is a valid sleep reason,
> technically also BLOCK_ON_RT_MUTEX is something you are allowing.
> 
> In my understanding, the contention tracepoints already in the kernel
> can track all contention by kernel code and are leaving aside the PI
> futexes, which use the untracked rt_mutex_wait_proxy_lock.
> 
> In your case, you are tracking PI_FUTEX via the system call, which
> should cover the above scenario.
>
> Do you really need extra tracepoints to track this too? Or is there any
> other use of start_proxy_lock/wait_proxy_lock I'm missing here?
> 
> I see the only case in which rt_mutex_start_proxy_lock is called with a
> task different than current is via FUTEX_CMP_REQUEUE_PI, wouldn't
> considering this one too make the new tracepoints superfluous (assuming
> this one is even needed to be tracked before FUTEX_WAIT_REQUEUE_PI).

The monitor allows PI_FUTEX and allows BLOCK_ON_RT_MUTEX in different
manners.

PI_FUTEX is only a valid sleep reason. If a task sleeps with PI_FUTEX=true,
it still has to obey ((not WAKE) until RT_FRIENDLY_WAKE)

On the other hand, BLOCK_ON_RT_MUTEX alone is good enough. Waker is not
checked due to how rt_mutex is implemented: when a task unlocks an rt_mutex
and wakes waiter, the task is priority-deboosted first before doing the
wakeup, and we would see a false positive warning. See
rt_mutex_slowunlock().

In the case of futex_lock_pi(), both PI_FUTEX and BLOCK_ON_RT_MUTEX is
true. Therefore we don't check the waker.

However, in the case of futex_wait_requeue_pi(), PI_FUTEX is true but
BLOCK_ON_RT_MUTEX is false. In this case, we check the waker.

So, what happens if we don't have the tracepoint in *_proxy_lock()? The
futex_lock_pi() may generate a false positive warning, because we check the
waker and the waker may have lower priority.

But now that you mention it, perhaps instead of PI_FUTEX, we could define
FUTEX_LOCK_PI and FUTEX_WAIT_REQUEUE_PI separately. And we don't check the
waker if FUTEX_LOCK_PI=true. Something like the diff below.

Then we wouldn't need the block_on_rt_mutex tracepoints anymore. And the
specification is a bit more obvious.

Having a second pair of eyes is great, thanks!

Nam

diff --git a/tools/verification/models/rtapp/sleep.ltl b/tools/verification/models/rtapp/sleep.ltl
index 6e2f1ff31163..1f26e58e72f8 100644
--- a/tools/verification/models/rtapp/sleep.ltl
+++ b/tools/verification/models/rtapp/sleep.ltl
@@ -3,7 +3,7 @@ RULE = always ((RT and SLEEP) imply (RT_FRIENDLY_SLEEP or ALLOWLIST))
 RT_FRIENDLY_SLEEP = (RT_VALID_SLEEP_REASON or KERNEL_THREAD)
                 and ((not WAKE) until RT_FRIENDLY_WAKE)
 
-RT_VALID_SLEEP_REASON = PI_FUTEX
+RT_VALID_SLEEP_REASON = FUTEX_WAIT_REQUEUE_PI
                      or RT_FRIENDLY_NANOSLEEP
 
 RT_FRIENDLY_NANOSLEEP = CLOCK_NANOSLEEP
@@ -16,5 +16,6 @@ RT_FRIENDLY_WAKE = WOKEN_BY_EQUAL_OR_HIGHER_PRIO
                 or KTHREAD_SHOULD_STOP
 
 ALLOWLIST = BLOCK_ON_RT_MUTEX
+         or FUTEX_LOCK_PI
          or TASK_IS_RCU
          or TASK_IS_MIGRATION