| Message ID | 20250206181711.1902989-9-elver@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Compiler-Based Capability- and Locking-Analysis | expand |
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. Bart.
On Mon, 10 Feb 2025 at 19:54, Bart Van Assche <bvanassche@acm.org> wrote: > > > On 2/10/25 10:23 AM, Marco Elver wrote: > > 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. > > In the above I see that two different options have been mentioned for > code that includes conditional lockdep_assert_held() calls: > - Either include __assert_cap() in the lockdep_assert_held() definition. > - Or annotate the entire function with __no_thread_safety_analysis. > > I think there is a third possibility: add an explicit __assert_cap() > call under the lockdep_assert_held() call. With this approach the > thread-safety analysis remains enabled for the annotated function and > the compiler will complain if neither __must_hold() nor __assert_cap() > has been used. That's just adding more clutter. Being able to leverage existing lockdep_assert to avoid false positives (at potential cost of few false negatives) is a decent trade-off. Sure, having maximum checking guarantees would be nice, but there's a balance we have to strike vs. ergonomics, usability, and pointless clutter. Can we initially try to avoid clutter as much as possible? Then, if you feel coverage is not good enough, make the analysis stricter by e.g. removing the implicit assert from lockdep_assert in later patches and see how it goes. I'm basing my judgement here on experience having worked on other analysis in the kernel, and the biggest request from maintainers has always been to "avoid useless clutter and false positives at all cost", often at the cost of increased potential for false negatives but avoiding false positives and reducing annotations (I can dig out discussions we had for KMSAN if you do not believe me...).
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) #define lockdep_assert_held_once(l) \ lockdep_assert_once(lockdep_is_held(l) != LOCK_STATE_NOT_HELD) @@ -389,10 +389,10 @@ extern int lockdep_is_held(const void *); #define lockdep_assert(c) do { } while (0) #define lockdep_assert_once(c) do { } while (0) -#define lockdep_assert_held(l) do { (void)(l); } while (0) +#define lockdep_assert_held(l) __assert_cap(l) #define lockdep_assert_not_held(l) do { (void)(l); } while (0) -#define lockdep_assert_held_write(l) do { (void)(l); } while (0) -#define lockdep_assert_held_read(l) do { (void)(l); } while (0) +#define lockdep_assert_held_write(l) __assert_cap(l) +#define lockdep_assert_held_read(l) __assert_shared_cap(l) #define lockdep_assert_held_once(l) do { (void)(l); } while (0) #define lockdep_assert_none_held_once() do { } while (0)
Clang's capability analysis can be made aware of functions that assert that capabilities/locks are held. 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. Signed-off-by: Marco Elver <elver@google.com> --- include/linux/lockdep.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)