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:
> > It would be interesting to see some patterns. One "trick" that I have
> > played with is to move the conditional locking into the capability
> > itself: it is then unconditionally acquired or released, but the
> > underlying mutex is only acquired or released based on some global
> > state or member. Something like this:
> >
> > struct __attribute__((capability("mutex"))) conditional_mutex
> > {
> >       struct mutex mu;
> >       bool active;
> > }
> >
> > void acquire_conditional_mutex(struct conditional_mutex* cmu)
> >       __attribute__((acquire_capability(cmu)))
> > {
> >       if (cmu->active)
> >               acquire_mutex(&cmu->mu);
> > }
> >
> > However, I realize that might be a difficult proposal for the kernel community.
> 
> Version of this that came up were:
> 
> static __always_inline u32
> bpf_prog_run_array_uprobe(const struct bpf_prog_array *array,
>                           const void *ctx, bpf_prog_run_fn run_prog)
> {
>         const struct bpf_prog_array_item *item;
>         const struct bpf_prog *prog;
>         struct bpf_run_ctx *old_run_ctx;
>         struct bpf_trace_run_ctx run_ctx;
>         u32 ret = 1;
> 
>         might_fault();
>         RCU_LOCKDEP_WARN(!rcu_read_lock_trace_held(), "no rcu lock held");
> 
>         if (unlikely(!array))
>                 return ret;
> 
>         migrate_disable();
> 
>         run_ctx.is_uprobe = true;
> 
>         old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>         item = &array->items[0];
>         while ((prog = READ_ONCE(item->prog))) {
>                 if (!prog->sleepable)
>                         rcu_read_lock();
> 
>                 run_ctx.bpf_cookie = item->bpf_cookie;
>                 ret &= run_prog(prog, ctx);
>                 item++;
> 
>                 if (!prog->sleepable)
>                         rcu_read_unlock();
>         }
>         bpf_reset_run_ctx(old_run_ctx);
>         migrate_enable();
>         return ret;
> }
> 
> and
> 
> static void ath9k_hif_usb_firmware_fail(struct hif_device_usb *hif_dev)
> {
>         struct device *dev = &hif_dev->udev->dev;
>         struct device *parent = dev->parent;
> 
>         complete_all(&hif_dev->fw_done);
> 
>         if (parent)
>                 device_lock(parent);
> 
>         device_release_driver(dev);
> 
>         if (parent)
>                 device_unlock(parent);
> }

So it boils down to

if (expr)
	acquire(mutex);
do_something();
if (expr)
	release(mutex);

If the "do_something()" block is rather short, as in the second example, it might also be an alternative to write:

         if (parent) {
                 device_lock(parent);
                 device_release_driver(dev);
                 device_unlock(parent);
         } else {
                 device_release_driver(dev);
         }

In any event, since the lock is only conditionally held, the block cannot require the mutex. So we can't be trying to satisfy "guarded_by" or "requires" attributes. We're just trying to avoid warnings about conditionally held capabilities. So at this point you might also be using "no_thread_safety_analysis". That's what we have typically done when we had this pattern.

> And I think we can expect a fair amount of resistance from maintainers
> if we're going to go around rewriting their -- perfectly fine code,
> thank you very much -- to please a static analyzer :/

I think there are some good arguments outside of the static analyzer. The pattern repeats the "if (expr)", and especially if "expr" is a bit more complex, this can be a source of errors:

                 if (!prog->sleepable)
                         rcu_read_lock();
 
                 // ...
 
                 if (prog->sleepable)
                         rcu_read_unlock();

This might slip through in a review, and could easily happen by copy-and-paste. (Of course you'll notice when testing this.) If you build some kind of conditional scope, it might look like this:

        scoped_conditional_guard (<rcu lock>, !prog->sleepable) {
                // ...
        }

Another aspect is that even if the condition is symbolically the same, its value might change between locking and unlocking. If you write the condition once and evaluate it once, you can exclude both sources of errors.

> One of the more common patterns that I've talked about with Marco is
> where modification requires two locks, while access requires either
> lock.
> 
> The current __guarded_by() annotation cannot express this.
> 
> One possible way would be to construct a fake / 0-size capability
> structure and have each of these locks acquire it in 'shared' mode (but
> then you run into the recursion thing) and have a special annotation
> that upgrades it to exclusive.
> 
> [...]
> 
> I would much rather write something like:
> 
> struct task_struct {
>         raw_spinlock_t pi_lock;
> 
>         cpumask_t       cpus_allowed __guarded_by(pi_lock, rq->__lock);
> }

There were ideas to support arbitrary Boolean expressions, but they wouldn't have allowed different expressions for read and write. So this would have been both too broad and not broad enough for your use case.

Your idea on the other hand sounds pretty reasonable to me. We already support multiple arguments in many other attributes, so it seems like a consistent extension. And semantically it makes sense to me as well: if a writer is required to hold all capabilities, then a reader should be fine if they hold any one. I'll have to think a bit more about whether this is always the right thing to do, but if it is, we have an easy way out.

> And now I realize this case is doubly annoying because (rq) isn't
> something that is in lexical scope at this point :-( These are the
> locking rules though, and it is vexing not being able to express things
> like this.

If it's not in scope, how do we know we have the right thing? Is task_struct always contained in some larger object that also has "rq", and never outside of that?

The language doesn't really prevent you from having a lonely task_struct, and then it wouldn't be clear what we're referring to.

A way out might be to consider the task_struct field in the larger object as guarded_by, and find a way to exclude the contained mutex. We could also introduce some kind of guarded_by_member(lock, member) to be added in the larger object. This would say that "lock" guards only the "member" subfield of the field. However, both would only apply if we're accessing the member through the parent. If you hand the task_struct by itself to some function and let that access cpus_allowed, we don't know what you're talking about.

But maybe you can explain a bit how the objects are coupled before I speculate too much into this.

Aaron