Re: [PATCH v2 07/12] rbd: Enable lock context analysis

[Nilay Shroff <nilay@linux.ibm.com>]

On 8/4/26 1:16 AM, Bart Van Assche wrote:
> On 8/3/26 7:03 AM, Nilay Shroff wrote:
>> In addition to the above annotations, it looks like several variables and structure members
>> are consistently protected by specific locks:
>>
>> 1. rbd_dev_list is guarded by rbd_dev_list_lock.
>> 2. rbd_client_list is guarded by rbd_client_list_lock.
>> 3. rbd_device::acquiring_list and rbd_device::running_list are guarded by
>>     rbd_device::lock_lists_lock.
>> 4. rbd_device::object_map is guarded by rbd_device::object_map_lock.
>> 5. rbd_device::watch_cookie, rbd_device::watch_state, and rbd_device::watch_handle
>>     are guarded by rbd_device::watch_mutex.
>> 6. rbd_device::lock_state and rbd_device::lock_cookie are guarded by rbd_device::lock_rwsem.
>>
>> So I think we should add the corresponding `__guarded_by(...)` annotations for these
>> fields as part of enabling lock context analysis for this driver.
> 
> Hi Nilay,
> 
> Thank you for having looked this up. I took a closer look at the rbd
> driver and came up with the patch below. Some observations:
> - Some code obtains pointers to acquiring_list, running_list and
>    object_map before the corresponding synchronization objects are
>    acquired. Hence, I have not tried to annotate these struct members
>    with __guarded_by().
> - The locking convention followed by rbd_img_object_requests() is not
>    a good fit for what Clang supports.
> 
> Further feedback is welcome.
> 
> Thanks,
> 
> Bart.
> 
> These are the changes I came up with (relative to the above patch):
> 
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index f0f7a94e3e3e..4c9f566b7a91 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -406,26 +406,26 @@ struct rbd_device {
>       struct mutex        watch_mutex;
>       enum rbd_watch_state    watch_state;
>       struct ceph_osd_linger_request *watch_handle;
> -    u64            watch_cookie;
> +    u64            watch_cookie __guarded_by(&watch_mutex);
>       struct delayed_work    watch_dwork;
> 
>       struct rw_semaphore    lock_rwsem;
> -    enum rbd_lock_state    lock_state;
> -    char            lock_cookie[32];
> +    enum rbd_lock_state    lock_state __guarded_by(&lock_rwsem);
> +    char            lock_cookie[32] __guarded_by(&lock_rwsem);
>       struct rbd_client_id    owner_cid;
>       struct work_struct    acquired_lock_work;
>       struct work_struct    released_lock_work;
>       struct delayed_work    lock_dwork;
>       struct work_struct    unlock_work;
>       spinlock_t        lock_lists_lock;
> -    struct list_head    acquiring_list;
> -    struct list_head    running_list;
> +    struct list_head    acquiring_list; /* __guarded_by(&lock) */
> +    struct list_head    running_list; /* __guarded_by(&lock) */
>       struct completion    acquire_wait;
>       int            acquire_err;
>       struct completion    quiescing_wait;
> 
>       spinlock_t        object_map_lock;
> -    u8            *object_map;
> +    u8            *object_map; /*__guarded_by(&object_map_lock)*/
>       u64            object_map_size;    /* in objects */
>       u64            object_map_flags;
> 
> @@ -464,11 +464,13 @@ enum rbd_dev_flags {
> 
>   static DEFINE_MUTEX(client_mutex);    /* Serialize client creation */
> 
> -static LIST_HEAD(rbd_dev_list);    /* devices */
> +/* devices */
>   static DEFINE_SPINLOCK(rbd_dev_list_lock);
> +static __guarded_by(&rbd_dev_list_lock) LIST_HEAD(rbd_dev_list);
> 
> -static LIST_HEAD(rbd_client_list);        /* clients */
> +/* clients */
>   static DEFINE_SPINLOCK(rbd_client_list_lock);
> +static __guarded_by(&rbd_client_list_lock) LIST_HEAD(rbd_client_list);
> 
>   /* Slab caches for frequently-allocated structures */
> 
> @@ -521,6 +523,7 @@ static bool rbd_is_snap(struct rbd_device *rbd_dev)
>   }
> 
>   static bool __rbd_is_lock_owner(struct rbd_device *rbd_dev)
> +    __must_hold_shared(&rbd_dev->lock_rwsem)
>   {
>       lockdep_assert_held(&rbd_dev->lock_rwsem);
> 
> @@ -1645,6 +1648,7 @@ static void __rbd_object_map_index(struct rbd_device *rbd_dev, u64 objno,
>   }
> 
>   static u8 __rbd_object_map_get(struct rbd_device *rbd_dev, u64 objno)
> +    __must_hold(&rbd_dev->object_map_lock)
>   {
>       u64 index;
>       u8 shift;
> @@ -1655,6 +1659,7 @@ static u8 __rbd_object_map_get(struct rbd_device *rbd_dev, u64 objno)
>   }
> 
>   static void __rbd_object_map_set(struct rbd_device *rbd_dev, u64 objno, u8 val)
> +    __must_hold(&rbd_dev->object_map_lock)
>   {
>       u64 index;
>       u8 shift;
> @@ -3431,6 +3436,7 @@ static bool need_exclusive_lock(struct rbd_img_request *img_req)
>   }
> 
>   static bool rbd_lock_add_request(struct rbd_img_request *img_req)
> +    __must_hold_shared(&img_req->rbd_dev->lock_rwsem)
>   {
>       struct rbd_device *rbd_dev = img_req->rbd_dev;
>       bool locked;
> @@ -3448,6 +3454,7 @@ static bool rbd_lock_add_request(struct rbd_img_request *img_req)
>   }
> 
>   static void rbd_lock_del_request(struct rbd_img_request *img_req)
> +    __must_hold_shared(&img_req->rbd_dev->lock_rwsem)
>   {
>       struct rbd_device *rbd_dev = img_req->rbd_dev;
>       bool need_wakeup = false;
> @@ -3472,6 +3479,8 @@ static int rbd_img_exclusive_lock(struct rbd_img_request *img_req)
>       if (!need_exclusive_lock(img_req))
>           return 1;
> 
> +    __assume_ctx_lock(&img_req->rbd_dev->lock_rwsem);
> +
>       if (rbd_lock_add_request(img_req))
>           return 1;
> 
> @@ -3490,6 +3499,12 @@ static void rbd_img_object_requests(struct rbd_img_request *img_req)
>       struct rbd_obj_request *obj_req;
> 
>       rbd_assert(!img_req->pending.result && !img_req->pending.num_pending);
> +    /*
> +     * The caller either obtains an exclusive lock or obtains
> +     * rbd_dev->lock_rwsem. Since this cannot be represented accurately
> +     * using lock context annotations, add an __assume_ctx_lock() statement.
> +     */
> +    __assume_ctx_lock(&rbd_dev->lock_rwsem);
>       rbd_assert(!need_exclusive_lock(img_req) ||
>              __rbd_is_lock_owner(rbd_dev));
> 
> @@ -3533,6 +3548,7 @@ static void rbd_img_object_requests(struct rbd_img_request *img_req)
>   }
> 
>   static bool rbd_img_advance(struct rbd_img_request *img_req, int *result)
> +    __must_hold(&img_req->state_mutex)
>   {
>       int ret;
> 
> @@ -3651,6 +3667,7 @@ static struct rbd_client_id rbd_get_cid(struct rbd_device *rbd_dev)
>    */
>   static void rbd_set_owner_cid(struct rbd_device *rbd_dev,
>                     const struct rbd_client_id *cid)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       dout("%s rbd_dev %p %llu-%llu -> %llu-%llu\n", __func__, rbd_dev,
>            rbd_dev->owner_cid.gid, rbd_dev->owner_cid.handle,
> @@ -3666,6 +3683,7 @@ static void format_lock_cookie(struct rbd_device *rbd_dev, char *buf)
>   }
> 
>   static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct rbd_client_id cid = rbd_get_cid(rbd_dev);
> 
> @@ -3679,6 +3697,7 @@ static void __rbd_lock(struct rbd_device *rbd_dev, const char *cookie)
>    * lock_rwsem must be held for write
>    */
>   static int rbd_lock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
>       char cookie[32];
> @@ -3702,6 +3721,7 @@ static int rbd_lock(struct rbd_device *rbd_dev)
>    * lock_rwsem must be held for write
>    */
>   static void rbd_unlock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
>       int ret;
> @@ -3840,6 +3860,7 @@ static int rbd_request_lock(struct rbd_device *rbd_dev)
>    * (i.e. "rbd map").
>    */
>   static void wake_lock_waiters(struct rbd_device *rbd_dev, int result)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct rbd_img_request *img_req;
> 
> @@ -3950,6 +3971,7 @@ static struct ceph_locker *get_lock_owner_info(struct rbd_device *rbd_dev)
> 
>   static int find_watcher(struct rbd_device *rbd_dev,
>               const struct ceph_locker *locker)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_osd_client *osdc = &rbd_dev->rbd_client->client->osdc;
>       struct ceph_watch_item *watchers;
> @@ -3999,6 +4021,7 @@ static int find_watcher(struct rbd_device *rbd_dev,
>    * lock_rwsem must be held for write
>    */
>   static int rbd_try_lock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       struct ceph_client *client = rbd_dev->rbd_client->client;
>       struct ceph_locker *locker, *refreshed_locker;
> @@ -4217,6 +4240,7 @@ static void rbd_pre_release_action(struct rbd_device *rbd_dev)
>   }
> 
>   static void __rbd_release_lock(struct rbd_device *rbd_dev)
> +    __must_hold(&rbd_dev->lock_rwsem)
>   {
>       rbd_assert(list_empty(&rbd_dev->running_list));
> 
> @@ -4256,6 +4280,7 @@ static void rbd_release_lock_work(struct work_struct *work)
>   }
> 
>   static void maybe_kick_acquire(struct rbd_device *rbd_dev)
> +    __must_hold_shared(&rbd_dev->lock_rwsem)
>   {
>       bool have_requests;
> 
> @@ -5302,6 +5327,7 @@ static void rbd_spec_free(struct kref *kref)
>   }
> 
>   static void rbd_dev_free(struct rbd_device *rbd_dev)
> +    __context_unsafe(cleanup function that does not need locking)
>   {
>       WARN_ON(rbd_dev->watch_state != RBD_WATCH_STATE_UNREGISTERED);
>       WARN_ON(rbd_dev->lock_state != RBD_LOCK_STATE_UNLOCKED);
> @@ -5338,6 +5364,7 @@ static void rbd_dev_release(struct device *dev)
>   }
> 
>   static struct rbd_device *__rbd_dev_create(struct rbd_spec *spec)
> +    __context_unsafe(initialization function that does not need locking)
>   {
>       struct rbd_device *rbd_dev;
> 
> 
The above change look good.

However, I still think acquiring_list, running_list, and object_map should
ideally be guarded by their respective locks. The fact that some code obtains
pointers to these objects before acquiring the corresponding lock makes the
locking pattern worth looking at more closely rather than simply leaving
these fields unannotated.

Could you perhaps Cc the RBD maintainers and mailing list on the next revision
and ask for their feedback on these locking patterns? They may be able to clarify
whether obtaining these pointers outside the lock is intentional and safe, or
whether there are existing races that have gone unnoticed.

Thanks,
--Nilay