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

Bart Van Assche
2025-03-05T15:28:19Z

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:

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 248416ecd01c..0d011270e642 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -945,15 +945,19 @@ static inline unsigned int
blk_boundary_sectors_left(sector_t offset,
*/
static inline struct queue_limits
queue_limits_start_update(struct request_queue *q)
+ ACQUIRE(q->limits_lock)
{
mutex_lock(&q->limits_lock);
return q->limits;
}
int queue_limits_commit_update_frozen(struct request_queue *q,
- struct queue_limits *lim);
+ struct queue_limits *lim)
+ RELEASE(q->limits_lock);
int queue_limits_commit_update(struct request_queue *q,
- struct queue_limits *lim);
-int queue_limits_set(struct request_queue *q, struct queue_limits *lim);
+ struct queue_limits *lim)
+ RELEASE(q->limits_lock);
+int queue_limits_set(struct request_queue *q, struct queue_limits *lim)
+ EXCLUDES(q->limits_lock);
int blk_validate_limits(struct queue_limits *lim);

/**
@@ -965,6 +969,7 @@ int blk_validate_limits(struct queue_limits *lim);
* starting update.
*/
static inline void queue_limits_cancel_update(struct request_queue *q)
+ RELEASE(q->limits_lock)
{
mutex_unlock(&q->limits_lock);
}

> 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:

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);
}

Thanks,

Bart.