RE: Thread Safety Analysis and the Linux kernel

> From: Peter Zijlstra <[email protected]>
> On Wed, Mar 05, 2025 at 11:54:14PM +0000, Puchert, Aaron wrote:
> > > 3. Ability to refer to locks in returned reference/pointer. For example:
> > >     struct foo *ret_lock_struct(void) ACQUIRE(return->somelock);
> > >     struct foo *try_ret_lock_struct(void) TRY_ACQUIRE(1,
> > > return->somelock); // locked if non-NULL
> > > I expect this also requires basic alias analysis to work so that
> > > assigning the returned pointer to a function-local variable and then
> > > later use in an unlock function works as expected.
> >
> > We had a discussion about this, maybe I can find it later. The idea
> > was to introduce a builtin for the return value. A rough outline:
> > * Introduce the builtin to the parser, say __builtin_return_value().
> > This shouldn't be hard.
> > * Add handling to Sema: we can only accept the builtin in attributes
> > on functions. The return type is the return type of the function.
> > * In the caller we might not need alias analysis: there are no
> > variables that alias. We already produce S-expressions for some return
> > values from functions, and then handle a DeclStmt with initializer. (I
> > introduced that for some C++ patterns in
> > https://reviews.llvm.org/D129755.)
> > * In the function itself we need to substitute the return expression
> > for the builtin before checking the exit set. (I assume this restricts
> > the possible attributes to acquire-type attributes.)
> >
> > We can also discuss this in more detail if you want to pick it up. It
> > might be quite a bit of work, but it should fit nicely into the
> > existing framework.
> 
> Yes, as far as I can follow, that should work nicely.
> 
> The thing I tried to work around yesterday, making clang ICE, was
> something like this function:
> 
> struct rq *__task_rq_lock(struct task_struct *p)
>         __must_hold(p->pi_lock)
>         __acquires(_Return->__lock)
> {
>         struct rq *rq;
> 
>         lockdep_assert_held(&p->pi_lock);
> 
>         for (;;) {
>                 rq = task_rq(p);
>                 raw_spin_rq_lock(rq);
>                 if (likely(rq == task_rq(p) && !task_on_rq_migrating(p)))
>                         return rq;
> 
>                 raw_spin_rq_unlock(rq);
> 
>                 while (unlikely(task_on_rq_migrating(p)))
>                         cpu_relax();
>         }
> }

This should fit well. We take the return expression "rq", plug it in for _Return in the attribute expression and get "rq->__lock". Then we compare this against the actual lockset, which should contain the same expression from the call "raw_spin_rq_lock(rq)".

> Users would typically look like:
> 
> try_to_wake_up(p, state)
> {
>         struct rq *rq;
> 
>         scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {
>                 if (!ttwu_state_match(p, state))
>                         break;
> 
>                 rq = __task_rq_lock(p);
>                 // go enqueue task
>                 raw_spin_rq_unlock(rq);
>         }
> }

Can the return value be used as an initializer by moving the declaration into the scoped_guard block? Or do you have a style guide that wants all declarations at the beginning of a block?

We track capabilities as symbolic expressions, so something like "rq->__lock" in this case. If there is an assignment to "rq", that changes the meaning of the symbolic expression. The object referred to by the expression is then no longer reachable. Currently we don't look at assignments at all when it comes to tracking capabilities. We don't even warn, it's simply documented as not being supported. We only look at initializers, as in alias analysis. Having a separate variable being initialized with the return value gives us a unique name for the return value, which is good when we're working with symbolic expressions.

If there are cases where assignment is really needed, we can also check to which extent we can rewrite expressions or warn when the objects they point to become unreachable. (In the example that would be the case if someone assigned to "rq" again after the call to "__task_rq_lock ".)