Re: [PATCH v2 00/34] Compiler-Based Capability- and Locking-Analysis

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Mar 05, 2025 at 07:27:32AM -0800, Bart Van Assche wrote:
> On 3/5/25 3:20 AM, Peter Zijlstra wrote:
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 248416ecd01c..d27607d9c2dc 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -945,6 +945,7 @@ static inline unsigned int blk_boundary_sectors_left(sector_t offset,
> >    */
> >   static inline struct queue_limits
> >   queue_limits_start_update(struct request_queue *q)
> > +	__acquires(q->limits_lock)
> >   {
> >   	mutex_lock(&q->limits_lock);
> >   	return q->limits;
> > @@ -965,6 +966,7 @@ int blk_validate_limits(struct queue_limits *lim);
> >    * starting update.
> >    */
> >   static inline void queue_limits_cancel_update(struct request_queue *q)
> > +	__releases(q->limits_lock)
> >   {
> >   	mutex_unlock(&q->limits_lock);
> >   }
> 
> The above is incomplete. Here is what I came up with myself:

Oh, I'm sure. I simply fixed whatever was topmost in the compile output
when trying to build kernel/sched/. After fixing these two, it stopped
complaining about blkdev.

I think it complains about these because they're inline, even though
they're otherwise unused.

> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 80a5b3268986..283fb85d96c8 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -1026,21 +1026,25 @@ static inline bool dev_pm_test_driver_flags(struct device *dev, u32 flags)
> >   }
> >   static inline void device_lock(struct device *dev)
> > +	__acquires(dev->mutex)
> >   {
> >   	mutex_lock(&dev->mutex);
> >   }
> >   static inline int device_lock_interruptible(struct device *dev)
> > +	__cond_acquires(0, dev->mutex)
> >   {
> >   	return mutex_lock_interruptible(&dev->mutex);
> >   }
> >   static inline int device_trylock(struct device *dev)
> > +	__cond_acquires(true, dev->mutex)
> >   {
> >   	return mutex_trylock(&dev->mutex);
> >   }
> >   static inline void device_unlock(struct device *dev)
> > +	__releases(dev->mutex)
> >   {
> >   	mutex_unlock(&dev->mutex);
> >   }
> 
> I propose to annotate these functions with __no_capability_analysis as a
> first step. Review of all callers of these functions in the entire
> kernel tree learned me that annotating these functions results in a
> significant number of false positives and not to the discovery of any
> bugs. The false positives are triggered by conditional locking. An
> example of code that triggers false positive thread-safety warnings:

Yeah, I've ran into this as well. The thing is entirely stupid when it
sees a branch. This is really unfortunate. But I disagree, I would
annotate those functions that have conditional locking with
__no_capability_analysis, or possibly:

#define __confused_by_conditionals __no_capability_analysis

I'm also not quite sure how to annotate things like pte_lockptr().


Anyway, this thing has some promise, however it is *really*, as in
*really* *REALLY* simple. Anything remotely interesting, where you
actually want the help, it falls over.

But you gotta start somewhere I suppose. I think the thing that is
important here is how receptive the clang folks are to working on this
-- because it definitely needs work.