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.
