On Mon, 10 Feb 2025 at 19:10, Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> On 2/6/25 10:10 AM, Marco Elver wrote:
> > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> > index 67964dc4db95..5cea929b2219 100644
> > --- a/include/linux/lockdep.h
> > +++ b/include/linux/lockdep.h
> > @@ -282,16 +282,16 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
> > do { WARN_ON_ONCE(debug_locks && !(cond)); } while (0)
> >
> > #define lockdep_assert_held(l) \
> > - lockdep_assert(lockdep_is_held(l) != LOCK_STATE_NOT_HELD)
> > + do { lockdep_assert(lockdep_is_held(l) != LOCK_STATE_NOT_HELD); __assert_cap(l); } while (0)
> >
> > #define lockdep_assert_not_held(l) \
> > lockdep_assert(lockdep_is_held(l) != LOCK_STATE_HELD)
> >
> > #define lockdep_assert_held_write(l) \
> > - lockdep_assert(lockdep_is_held_type(l, 0))
> > + do { lockdep_assert(lockdep_is_held_type(l, 0)); __assert_cap(l); } while (0)
> >
> > #define lockdep_assert_held_read(l) \
> > - lockdep_assert(lockdep_is_held_type(l, 1))
> > + do { lockdep_assert(lockdep_is_held_type(l, 1)); __assert_shared_cap(l); } while (0)
>
> These changes look wrong to me. The current behavior of
> lockdep_assert_held(lock) is that it issues a kernel warning at
> runtime if `lock` is not held when a lockdep_assert_held()
> statement is executed. __assert_cap(lock) tells the compiler to
> *ignore* the absence of __must_hold(lock). I think this is wrong.
> The compiler should complain if a __must_hold(lock) annotation is
> missing. While sparse does not support interprocedural analysis for
> lock contexts, the Clang thread-safety checker supports this. If
> function declarations are annotated with __must_hold(lock), Clang will
> complain if the caller does not hold `lock`.
>
> In other words, the above changes disable a useful compile-time check.
> I think that useful compile-time checks should not be disabled.
The assert_capability attribute was designed precisely for assertions
that check at runtime that the lock is held, and delegate to runtime
verification where the static analysis is just not powerful enough. In
the commit description:
Presence of these annotations causes the analysis to assume the
capability is held after calls to the annotated function, and avoid
false positives with complex control-flow; for example, where not all
control-flow paths in a function require a held lock, and therefore
marking the function with __must_hold(..) is inappropriate.
If you try to write code where you access a guarded_by variable, but
the lock is held not in all paths we can write it like this:
struct bar {
spinlock_t lock;
bool a; // true if lock held
int counter __var_guarded_by(&lock);
};
void foo(struct bar *d)
{
...
if (d->a) {
lockdep_assert_held(&d->lock);
d->counter++;
} else {
// lock not held!
}
...
}
Without lockdep_assert_held() you get false positives, and there's no
other good way to express this if you do not want to always call foo()
with the lock held.
It essentially forces addition of lockdep checks where the static
analysis can't quite prove what you've done is right. This is
desirable over adding no-analysis attributes and not checking anything
at all.
