Enable lock context analysis in drivers/block/

Add a missing __must_hold() annotation. Enable lock context analysis in the Makefile.

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/aoe/Makefile | 2 ++
  drivers/block/aoe/aoecmd.c | 1 +
  2 files changed, 3 insertions(+)

drivers/block/aoe/Makefile +2 -0

index b7545ce2f1b0..27bff6359a56 100644
@@ -3,5 +3,7 @@
  # Makefile for ATA over Ethernet
  #
 
+CONTEXT_ANALYSIS := y
+
  obj-$(CONFIG_ATA_OVER_ETH)	+= aoe.o
  aoe-y := aoeblk.o aoechr.o aoecmd.o aoedev.o aoemain.o aoenet.o

drivers/block/aoe/aoecmd.c +0 -0

index a4744a30a8af..54c57b9f8894 100644

As I see, iocq->lock guards iocq->head. So shouldn't we annotate iocq->head with __guarded_by(iocq->lock)? For instance,

struct iocq_ktio {
    struct list_head head;
                __guarded_by(&lock);
    spinlock_t lock;
};

Quoted 8 lines · Nilay Shroff

As I see, iocq->lock guards iocq->head. So shouldn't we annotate iocq->head with __guarded_by(iocq->lock)? For instance,

struct iocq_ktio {
     struct list_head head;
                 __guarded_by(&lock);
     spinlock_t lock;
};

How about integrating the change below into this patch?

drivers/block/aoe/aoecmd.c +1 -1

index 54c57b9f8894..7ceda4ee4e55 100644
@@ -51,8 +51,8 @@ static struct ktstate *kts;
 
  /* io completion queue */
  struct iocq_ktio {
-    struct list_head head;
      spinlock_t lock;
+    struct list_head head __guarded_by(&lock);
  };
  static struct iocq_ktio *iocq;
 

Move the code that is protected by a mutex from loop_change_fd() into __loop_change_fd(). Move the code that is protected by a mutex from loop_configure() into __loop_configure(). Keep the behavior in loop_global_lock_killable() for the global == true case. Expand loop_global_lock_killable(lo, false) calls into a mutex_lock_killable() and a mutex_unlock() call. This patch prepares for adding lock context annotations.

Cc Haris Iqbal <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/loop.c | 249 ++++++++++++++++++++++++-------------------
  1 file changed, 138 insertions(+), 111 deletions(-)

drivers/block/loop.c +137 -110

index 1faecef33009..a71fe763c933 100644
@@ -98,25 +98,22 @@ static DEFINE_MUTEX(loop_validate_mutex);
   * loop_global_lock_killable() - take locks for safe loop_validate_file() test
   *
   * @lo: struct loop_device
- * @global: true if @lo is about to bind another "struct loop_device", false otherwise
   *
   * Returns 0 on success, -EINTR otherwise.
   *
- * Since loop_validate_file() traverses on other "struct loop_device" if
- * is_loop_device() is true, we need a global lock for serializing concurrent
+ * Since loop_validate_file() traverses on other "struct loop_device", we need a
+ * global lock for serializing concurrent
   * loop_configure()/loop_change_fd()/__loop_clr_fd() calls.
   */
-static int loop_global_lock_killable(struct loop_device *lo, bool global)
+static int loop_global_lock_killable(struct loop_device *lo)
  {
  	int err;
 
-	if (global) {
-		err = mutex_lock_killable(&loop_validate_mutex);
-		if (err)
-			return err;
-	}
+	err = mutex_lock_killable(&loop_validate_mutex);
+	if (err)
+		return err;
  	err = mutex_lock_killable(&lo->lo_mutex);
-	if (err && global)
+	if (err)
  		mutex_unlock(&loop_validate_mutex);
  	return err;
  }

@@ -125,13 +122,11 @@ static int loop_global_lock_killable(struct loop_device *lo, bool global)
   * loop_global_unlock() - release locks taken by loop_global_lock_killable()
   *
   * @lo: struct loop_device
- * @global: true if @lo was about to bind another "struct loop_device", false otherwise
   */
-static void loop_global_unlock(struct loop_device *lo, bool global)
+static void loop_global_unlock(struct loop_device *lo)
  {
  	mutex_unlock(&lo->lo_mutex);
-	if (global)
-		mutex_unlock(&loop_validate_mutex);
+	mutex_unlock(&loop_validate_mutex);
  }
 
  static int max_part;

@@ -523,6 +518,50 @@ static int loop_check_backing_file(struct file *file)
  	return 0;
  }
 
+static int __loop_change_fd(struct loop_device *lo, struct block_device *bdev,
+			    struct file *file, struct file **old_file,
+			    bool *partscan)
+	__must_hold(&lo->lo_mutex)
+{
+	unsigned int memflags;
+	int error;
+
+	if (lo->lo_state != Lo_bound)
+		return -ENXIO;
+
+	/* the loop device has to be read-only */
+	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
+		return -EINVAL;
+
+	error = loop_validate_file(file, bdev);
+	if (error)
+		return error;
+
+	*old_file = lo->lo_backing_file;
+
+	/* size of the new backing store needs to be the same */
+	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, *old_file))
+		return -EINVAL;
+
+	/*
+	 * We might switch to direct I/O mode for the loop device, write back
+	 * all dirty data the page cache now that so that the individual I/O
+	 * operations don't have to do that.
+	 */
+	vfs_fsync(file, 0);
+
+	/* and ... switch */
+	disk_force_media_change(lo->lo_disk);
+	memflags = blk_mq_freeze_queue(lo->lo_queue);
+	mapping_set_gfp_mask((*old_file)->f_mapping, lo->old_gfp_mask);
+	loop_assign_backing_file(lo, file);
+	loop_update_dio(lo);
+	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
+	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
+
+	return 0;
+}
+
  /*
   * loop_change_fd switched the backing store of a loopback device to
   * a new file. This is useful for operating system installers to free up

@@ -536,7 +575,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
  {
  	struct file *file = fget(arg);
  	struct file *old_file;
-	unsigned int memflags;
  	int error;
  	bool partscan;
  	bool is_loop;

@@ -554,46 +592,21 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
  	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
 
  	is_loop = is_loop_device(file);
-	error = loop_global_lock_killable(lo, is_loop);
+	if (is_loop) {
+		error = loop_global_lock_killable(lo);
+		if (error)
+			goto out_putf;
+		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
+		loop_global_unlock(lo);
+	} else {
+		error = mutex_lock_killable(&lo->lo_mutex);
+		if (error)
+			goto out_putf;
+		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
+		mutex_unlock(&lo->lo_mutex);
+	}
  	if (error)
  		goto out_putf;
-	error = -ENXIO;
-	if (lo->lo_state != Lo_bound)
-		goto out_err;
-
-	/* the loop device has to be read-only */
-	error = -EINVAL;
-	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
-		goto out_err;
-
-	error = loop_validate_file(file, bdev);
-	if (error)
-		goto out_err;
-
-	old_file = lo->lo_backing_file;
-
-	error = -EINVAL;
-
-	/* size of the new backing store needs to be the same */
-	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, old_file))
-		goto out_err;
-
-	/*
-	 * We might switch to direct I/O mode for the loop device, write back
-	 * all dirty data the page cache now that so that the individual I/O
-	 * operations don't have to do that.
-	 */
-	vfs_fsync(file, 0);
-
-	/* and ... switch */
-	disk_force_media_change(lo->lo_disk);
-	memflags = blk_mq_freeze_queue(lo->lo_queue);
-	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
-	loop_assign_backing_file(lo, file);
-	loop_update_dio(lo);
-	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
-	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
-	loop_global_unlock(lo, is_loop);
 
  	/*
  	 * Flush loop_validate_file() before fput(), for l->lo_backing_file

@@ -618,8 +631,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
  	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
  	return error;
 
-out_err:
-	loop_global_unlock(lo, is_loop);
  out_putf:
  	fput(file);
  	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);

@@ -974,61 +985,29 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
  		lim->discard_granularity = 0;
  }
 
-static int loop_configure(struct loop_device *lo, blk_mode_t mode,
-			  struct block_device *bdev,
-			  const struct loop_config *config)
+static int __loop_configure(struct loop_device *lo, blk_mode_t mode,
+			    struct block_device *bdev,
+			    const struct loop_config *config, struct file *file,
+			    bool *partscan)
+	__must_hold(&lo->lo_mutex)
  {
-	struct file *file = fget(config->fd);
  	struct queue_limits lim;
-	int error;
  	loff_t size;
-	bool partscan;
-	bool is_loop;
-
-	if (!file)
-		return -EBADF;
-
-	error = loop_check_backing_file(file);
-	if (error) {
-		fput(file);
-		return error;
-	}
-
-	is_loop = is_loop_device(file);
-
-	/* This is safe, since we have a reference from open(). */
-	__module_get(THIS_MODULE);
-
-	/*
-	 * If we don't hold exclusive handle for the device, upgrade to it
-	 * here to avoid changing device under exclusive owner.
-	 */
-	if (!(mode & BLK_OPEN_EXCL)) {
-		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
-		if (error)
-			goto out_putf;
-	}
-
-	error = loop_global_lock_killable(lo, is_loop);
-	if (error)
-		goto out_bdev;
+	int error;
 
-	error = -EBUSY;
  	if (lo->lo_state != Lo_unbound)
-		goto out_unlock;
+		return -EBUSY;
 
  	error = loop_validate_file(file, bdev);
  	if (error)
-		goto out_unlock;
+		return error;
 
-	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0) {
-		error = -EINVAL;
-		goto out_unlock;
-	}
+	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0)
+		return -EINVAL;
 
  	error = loop_set_status_from_info(lo, &config->info);
  	if (error)
-		goto out_unlock;
+		return error;
  	lo->lo_flags = config->info.lo_flags;
 
  	if (!(file->f_mode & FMODE_WRITE) || !(mode & BLK_OPEN_WRITE) ||

@@ -1039,10 +1018,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
  		lo->workqueue = alloc_workqueue("loop%d",
  						WQ_UNBOUND | WQ_FREEZABLE,
  						0, lo->lo_number);
-		if (!lo->workqueue) {
-			error = -ENOMEM;
-			goto out_unlock;
-		}
+		if (!lo->workqueue)
+			return -ENOMEM;
  	}
 
  	/* suppress uevents while reconfiguring the device */

@@ -1059,7 +1036,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
  	/* No need to freeze the queue as the device isn't bound yet. */
  	error = queue_limits_commit_update(lo->lo_queue, &lim);
  	if (error)
-		goto out_unlock;
+		return error;
 
  	/*
  	 * We might switch to direct I/O mode for the loop device, write back

@@ -1080,14 +1057,66 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
  	WRITE_ONCE(lo->lo_state, Lo_bound);
  	if (part_shift)
  		lo->lo_flags |= LO_FLAGS_PARTSCAN;
-	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
-	if (partscan)
+	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
+	if (*partscan)
  		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
 
  	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
  	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 
-	loop_global_unlock(lo, is_loop);
+	return 0;
+}
+
+static int loop_configure(struct loop_device *lo, blk_mode_t mode,
+			  struct block_device *bdev,
+			  const struct loop_config *config)
+{
+	struct file *file = fget(config->fd);
+	int error;
+	bool partscan;
+	bool is_loop;
+
+	if (!file)
+		return -EBADF;
+
+	error = loop_check_backing_file(file);
+	if (error) {
+		fput(file);
+		return error;
+	}
+
+	is_loop = is_loop_device(file);
+
+	/* This is safe, since we have a reference from open(). */
+	__module_get(THIS_MODULE);
+
+	/*
+	 * If we don't hold exclusive handle for the device, upgrade to it
+	 * here to avoid changing device under exclusive owner.
+	 */
+	if (!(mode & BLK_OPEN_EXCL)) {
+		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
+		if (error)
+			goto out_putf;
+	}
+
+	if (is_loop) {
+		error = loop_global_lock_killable(lo);
+		if (error)
+			goto out_bdev;
+		error = __loop_configure(lo, mode, bdev, config, file,
+					 &partscan);
+		loop_global_unlock(lo);
+	} else {
+		error = mutex_lock_killable(&lo->lo_mutex);
+		if (error)
+			goto out_bdev;
+		error = __loop_configure(lo, mode, bdev, config, file,
+					 &partscan);
+		mutex_unlock(&lo->lo_mutex);
+	}
+	if (error)
+		goto out_bdev;
  	if (partscan)
  		loop_reread_partitions(lo);
 

@@ -1096,8 +1125,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
 
  	return 0;
 
-out_unlock:
-	loop_global_unlock(lo, is_loop);
  out_bdev:
  	if (!(mode & BLK_OPEN_EXCL))
  		bd_abort_claiming(bdev, loop_configure);

@@ -1194,11 +1221,11 @@ static int loop_clr_fd(struct loop_device *lo)
  	 * which loop_configure()/loop_change_fd() found via fget() was this
  	 * loop device.
  	 */
-	err = loop_global_lock_killable(lo, true);
+	err = loop_global_lock_killable(lo);
  	if (err)
  		return err;
  	if (lo->lo_state != Lo_bound) {
-		loop_global_unlock(lo, true);
+		loop_global_unlock(lo);
  		return -ENXIO;
  	}
  	/*

Overall this change looks good to me. But I see, lo->lo_lock is used to protect lo->lo_backing_file. so shall we annotate the lo->lo_backing_file using __guarded_by(&lo_lock)?

Quoted 3 lines · Nilay Shroff

Overall this change looks good to me. But I see, lo->lo_lock is used to protect lo->lo_backing_file. so shall we annotate the lo->lo_backing_file using __guarded_by(&lo_lock)?

I don't think so. It seems to me that the strategy in the loop driver for serializing accesses to lo->lo_backing_file is too complicated for lock context annotations. My understanding is as follows:
* lo->lo_mutex serializes configuration and state changes on an individual loop device.
* loop_validate_mutex serializes concurrent loop_configure(), loop_change_fd(), and loop_clr_fd() calls across all loop devices to safely execute loop_validate_file() when loop devices are stacked or nested.
* The blk_mq_freeze_queue() call in __loop_change_fd() serializes I/O request processing and the code in __loop_change_fd() that is executed while the queue is frozen.

Add lock context annotations that are compatible with Clang. Swap the loop_ctl_mutex and loop_index_idr declarations such that __guarded_by() can be used.

Cc Nilay Shroff <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/loop.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/block/loop.c +0 -0

index a71fe763c933..e2ac19de11f4 100644

Quoted 15 lines · Bart Van Assche

Add lock context annotations that are compatible with Clang. Swap the loop_ctl_mutex and loop_index_idr declarations such that __guarded_by() can be used.

Cc Nilay Shroff <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/loop.c | 7 ++++++-

  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/block/loop.c +0 -0

index a71fe763c933..e2ac19de11f4 100644

drivers/block/loop.c struct loop_cmd { · line 93

Quoted 5 lines

drivers/block/loop.c +1 -1

@@ -90,8 +90,8 @@ struct loop_cmd {
   #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
   #define LOOP_DEFAULT_HW_Q_DEPTH 128
-static DEFINE_IDR(loop_index_idr);
   static DEFINE_MUTEX(loop_ctl_mutex);
+static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);

Can we define helper macro here, which is more readable?

# define DEFINE_IDR_GUARDED(name, lock)\
         __guarded_by(&(lock)) DEFINE_IDR(name);

This is similar to what we have defined for guarded LIST_HEAD here:
https://lore.kernel.org/all/[email protected]/

Quoted 24 lines · Nilay Shroff

Quoted 15 lines · Bart Van Assche

Add lock context annotations that are compatible with Clang. Swap the loop_ctl_mutex and loop_index_idr declarations such that __guarded_by() can be used.

Cc Nilay Shroff <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/loop.c | 7 ++++++-

  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/block/loop.c +0 -0

index a71fe763c933..e2ac19de11f4 100644

drivers/block/loop.c struct loop_cmd { · line 93

Quoted 5 lines

drivers/block/loop.c +1 -1

@@ -90,8 +90,8 @@ struct loop_cmd {
   #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
   #define LOOP_DEFAULT_HW_Q_DEPTH 128
-static DEFINE_IDR(loop_index_idr);
   static DEFINE_MUTEX(loop_ctl_mutex);
+static __guarded_by(&loop_ctl_mutex) DEFINE_IDR(loop_index_idr);

Can we define helper macro here, which is more readable?

Quoted 5 lines

# define DEFINE_IDR_GUARDED(name, lock)\
         __guarded_by(&(lock)) DEFINE_IDR(name);

This is similar to what we have defined for guarded LIST_HEAD here:
https://lore.kernel.org/all/[email protected]/

Maybe I missed something but it's not clear to me how such a short macro helps? It doesn't seem to improve code readability to me?

Let the compiler verify lock and unlock calls. Enable lock context analysis in the Makefile.

Reviewed-by Christoph Hellwig <[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/mtip32xx/Makefile | 2 ++
  1 file changed, 2 insertions(+)

drivers/block/mtip32xx/Makefile +0 -0

index bff32b5d3c19..233961fdb41b 100644

Add __must_hold() annotations where these are missing. Document which mutex protects nbd_index_idr.

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/nbd.c | 11 +++++++++--
  1 file changed, 9 insertions(+), 2 deletions(-)

drivers/block/nbd.c +0 -0

index 8f10762e90ef..751449d362b0 100644

drivers/block/nbd.c +3 -0

  static struct workqueue_struct *nbd_del_wq;
  static int nbd_total_devices = 0;
 

@@ -1506,6 +1506,7 @@ static void nbd_config_put(struct nbd_device *nbd)
  }
 
  static int nbd_start_device(struct nbd_device *nbd)
+	__must_hold(&nbd->config_lock)
  {
  	struct nbd_config *config = nbd->config;
  	int num_connections = config->num_connections;

@@ -1578,6 +1579,7 @@ static int nbd_start_device(struct nbd_device *nbd)
  }
 
  static int nbd_start_device_ioctl(struct nbd_device *nbd)
+	__must_hold(&nbd->config_lock)
  {
  	struct nbd_config *config = nbd->config;
  	int ret;

@@ -1629,6 +1631,7 @@ static void nbd_set_cmd_timeout(struct nbd_device *nbd, u64 timeout)
  /* Must be called with config_lock held */
  static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
  		       unsigned int cmd, unsigned long arg)
+	__must_hold(&nbd->config_lock)
  {
  	struct nbd_config *config = nbd->config;
  	loff_t bytesize;

drivers/block/nbd.c static void __exit nbd_cleanup(void) · line 2782

Quoted 9 lines · Bart Van Assche

drivers/block/nbd.c +5 -1

@@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
       /* Also wait for nbd_dev_remove_work() completes */
       destroy_workqueue(nbd_del_wq);
-    idr_destroy(&nbd_index_idr);
+    {
+        __assume_ctx_lock(&nbd_index_mutex);
+        idr_destroy(&nbd_index_idr);
+    }
+

Could we instead introduce a scoped __assume_ctx_lock() helper so we don't need the explicit scope here? Alternatively, would annotating nbd_cleanup() with __context_unsafe be a better fit?

Quoted 14 lines · Nilay Shroff

drivers/block/nbd.c static void __exit nbd_cleanup(void) · line 2782

Quoted 9 lines · Bart Van Assche

drivers/block/nbd.c +5 -1

@@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
       /* Also wait for nbd_dev_remove_work() completes */
       destroy_workqueue(nbd_del_wq);
-    idr_destroy(&nbd_index_idr);
+    {
+        __assume_ctx_lock(&nbd_index_mutex);
+        idr_destroy(&nbd_index_idr);
+    }
+

Could we instead introduce a scoped __assume_ctx_lock() helper so we don't need the explicit scope here? Alternatively, would annotating nbd_cleanup() with __context_unsafe be a better fit?

There is code in nbd_cleanup() that must be protected by a mutex so I think that we shouldn't annotate nbd_cleanup() with __context_unsafe().

Introducing a scoped __assume_ctx_lock() might be overkill.

Quoted 14 lines · Nilay Shroff

drivers/block/nbd.c static void __exit nbd_cleanup(void) · line 2782

Quoted 9 lines · Bart Van Assche

drivers/block/nbd.c +5 -1

@@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
      /* Also wait for nbd_dev_remove_work() completes */
      destroy_workqueue(nbd_del_wq);
-    idr_destroy(&nbd_index_idr);
+    {
+        __assume_ctx_lock(&nbd_index_mutex);
+        idr_destroy(&nbd_index_idr);
+    }
+

Could we instead introduce a scoped __assume_ctx_lock() helper so we don't need the explicit scope here? Alternatively, would annotating nbd_cleanup() with __context_unsafe be a better fit?

There is code in nbd_cleanup() that must be protected by a mutex so I think that we shouldn't annotate nbd_cleanup() with __context_unsafe().

Introducing a scoped __assume_ctx_lock() might be overkill.

Quoted 16 lines · Bart Van Assche

Quoted 14 lines · Nilay Shroff

drivers/block/nbd.c static void __exit nbd_cleanup(void) · line 2782

Quoted 9 lines · Bart Van Assche

drivers/block/nbd.c +5 -1

@@ -2773,7 +2776,11 @@ static void __exit nbd_cleanup(void)
       /* Also wait for nbd_dev_remove_work() completes */
       destroy_workqueue(nbd_del_wq);
-    idr_destroy(&nbd_index_idr);
+    {
+        __assume_ctx_lock(&nbd_index_mutex);
+        idr_destroy(&nbd_index_idr);
+    }
+

Could we instead introduce a scoped __assume_ctx_lock() helper so we don't need the explicit scope here? Alternatively, would annotating nbd_cleanup() with __context_unsafe be a better fit?

Quoted 2 lines

There is code in nbd_cleanup() that must be protected by a mutex so I think that we shouldn't annotate nbd_cleanup() with __context_unsafe().

Just 'context_unsafe(idr_destroy(&nbd_index_idr)); /* comment */' would be fine here I think. I've used that elsewhere for destructors. It makes sure that the rest of the code is still checked and is as simple as it gets.

Quoted 1 line

Introducing a scoped __assume_ctx_lock() might be overkill.

Agree. Note that __assume_ctx_lock() leaks outside the scope, so a real scoped __assume_ctx_lock() would just do no-op __acquire() and __release() using the cleanup.h infrastructure (similar to the init guards I suppose).

Add __must_hold() annotations where these are missing. Because null_lock_zone() and null_unlock_zone() use conditional locking, instead of annotating these functions, introduce guard class null_zone. Replace null_lock_zone() and null_unlock_zone() calls with scoped_guard(null_zone, ...). Enable lock context analysis in the Makefile.

Signed-off-by Bart Van Assche<[email protected]>

---
  drivers/block/null_blk/Makefile |   2 +
  drivers/block/null_blk/main.c   |   4 ++
  drivers/block/null_blk/zoned.c  | 120 ++++++++++++++------------------
  3 files changed, 57 insertions(+), 69 deletions(-)

drivers/block/null_blk/Makefile +2 -0

index 84c36e512ab8..282b0d51a477 100644
@@ -1,5 +1,7 @@
  # SPDX-License-Identifier: GPL-2.0
 
+CONTEXT_ANALYSIS := y
+
  # needed for trace events
  ccflags-y			+= -I$(src)
 

drivers/block/null_blk/main.c +4 -0

index f8c0fd57e041..baffc6c61178 100644
@@ -1038,6 +1038,7 @@ static struct nullb_page *null_insert_page(struct nullb *nullb,
  }
 
  static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page)
+	__must_hold(&nullb->lock)
  {
  	int i;
  	unsigned int offset;

@@ -1087,6 +1088,7 @@ static int null_flush_cache_page(struct nullb *nullb, struct nullb_page *c_page)
  }
 
  static int null_make_cache_space(struct nullb *nullb, unsigned long n)
+	__must_hold(&nullb->lock)
  {
  	int i, err, nr_pages;
  	struct nullb_page *c_pages[FREE_BATCH];

@@ -1141,6 +1143,7 @@ static int null_make_cache_space(struct nullb *nullb, unsigned long n)
 
  static blk_status_t copy_to_nullb(struct nullb *nullb, void *source,
  				  loff_t pos, size_t n, bool is_fua)
+	__must_hold(&nullb->lock)
  {
  	size_t temp, count = 0;
  	struct nullb_page *t_page;

@@ -1242,6 +1245,7 @@ static blk_status_t null_handle_flush(struct nullb *nullb)
  static blk_status_t null_transfer(struct nullb *nullb, struct page *page,
  	unsigned int len, unsigned int off, bool is_write, loff_t pos,
  	bool is_fua)
+	__must_hold(&nullb->lock)
  {
  	struct nullb_device *dev = nullb->dev;
  	blk_status_t err = BLK_STS_OK;

drivers/block/null_blk/zoned.c +10 -17

index 384bdce6a9b7..dbae748c90ba 100644
@@ -30,23 +30,26 @@ static inline void null_init_zone_lock(struct nullb_device *dev,
  		mutex_init(&zone->mutex);
  }
 
-static inline void null_lock_zone(struct nullb_device *dev,
-				  struct nullb_zone *zone)
-{
-	if (!dev->memory_backed)
-		spin_lock_irq(&zone->spinlock);
-	else
-		mutex_lock(&zone->mutex);
-}
-
-static inline void null_unlock_zone(struct nullb_device *dev,
-				    struct nullb_zone *zone)
-{
-	if (!dev->memory_backed)
-		spin_unlock_irq(&zone->spinlock);
-	else
-		mutex_unlock(&zone->mutex);
-}
+struct nullb_dev_and_zone {
+	struct nullb_device *dev;
+	struct nullb_zone *zone;
+};
+
+DEFINE_CLASS(null_zone, struct nullb_dev_and_zone, ({
+		     if (!_T.dev->memory_backed)
+			     spin_unlock_irq(&_T.zone->spinlock);
+		     else
+			     mutex_unlock(&_T.zone->mutex);

Quoted 9 lines · Bart Van Assche

+DEFINE_CLASS(null_zone, struct nullb_dev_and_zone, ({ +             if (!_T.dev->memory_backed)
+                 spin_unlock_irq(&_T.zone->spinlock); +             else
+                 mutex_unlock(&_T.zone->mutex); +         }), ({
+             if (!dev->memory_backed)
+                 spin_lock_irq(&zone->spinlock); +             else

Review

Quoted 6 lines

+                 mutex_lock(&zone->mutex);
+             (struct nullb_dev_and_zone){ dev, zone }; +         }),
+         struct nullb_device *dev, struct nullb_zone *zone)

(quoted diff) +2 -0

+
+DEFINE_CLASS_IS_UNCONDITIONAL(null_zone)

What do we gain by introducing a guard class here?

Since both the constructor and destructor are marked __context_unsafe, the lock context analysis still won't reason about the conditional locking.

I'm not opposed to using a guard class here, but it does make the code a bit more complex to follow.

Quoted 10 lines · Nilay Shroff

Quoted 9 lines · Bart Van Assche

+DEFINE_CLASS(null_zone, struct nullb_dev_and_zone, ({ +             if (!_T.dev->memory_backed)
+                 spin_unlock_irq(&_T.zone->spinlock); +             else
+                 mutex_unlock(&_T.zone->mutex); +         }), ({
+             if (!dev->memory_backed)
+                 spin_lock_irq(&zone->spinlock); +             else

Quoted 8 lines

Review

Quoted 6 lines

+                 mutex_lock(&zone->mutex);
+             (struct nullb_dev_and_zone){ dev, zone }; +         }),
+         struct nullb_device *dev, struct nullb_zone *zone)

(quoted diff) +2 -0

+
+DEFINE_CLASS_IS_UNCONDITIONAL(null_zone)

What do we gain by introducing a guard class here?

What we gain is that __context_unsafe() annotations are avoided entirely. __context_unsafe() is a big hammer that shouldn't be used unless absolutely necessary. Additionally, introducing this class makes it possible to convert multiple goto statements into return statements. I think that's a significant improvement.

Quoted 3 lines

Since both the constructor and destructor are marked __context_unsafe, the lock context analysis still won't reason about the conditional locking.

Such reasoning is not needed when using guard() or scoped_guard() since these macro's make it impossible to leave a scope without releasing the acquired synchronization object.

Quoted 2 lines

I'm not opposed to using a guard class here, but it does make the code a bit more complex to follow.

Hmm ... I think this means that you are not familiar yet with DEFINE_CLASS(). The code that uses the null_zone class becomes simpler.

Add lock context annotations.

Signed-off-by Bart Van Assche <[email protected]>

---
  drivers/block/rbd.c | 8 ++++++++
  1 file changed, 8 insertions(+)

drivers/block/rbd.c +5 -0

index 1f1c2810f6ee..f0f7a94e3e3e 100644
@@ -4183,6 +4183,7 @@ static void rbd_acquire_lock(struct work_struct *work)
  }
 
  static bool rbd_quiesce_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	dout("%s rbd_dev %p\n", __func__, rbd_dev);
  	lockdep_assert_held_write(&rbd_dev->lock_rwsem);

@@ -4227,6 +4228,7 @@ static void __rbd_release_lock(struct rbd_device *rbd_dev)
   * lock_rwsem must be held for write
   */
  static void rbd_release_lock(struct rbd_device *rbd_dev)
+	__must_hold(&rbd_dev->lock_rwsem)
  {
  	if (!rbd_quiesce_lock(rbd_dev))
  		return;

@@ -4583,6 +4585,7 @@ static void rbd_unregister_watch(struct rbd_device *rbd_dev)
   * lock_rwsem must be held for write
   */
  static void rbd_reacquire_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];

@@ -6780,6 +6783,7 @@ static void rbd_dev_device_release(struct rbd_device *rbd_dev)
   * upon return.
   */
  static int rbd_dev_device_setup(struct rbd_device *rbd_dev)
+	__releases(&rbd_dev->header_rwsem)
  {
  	int ret;
 

@@ -6881,6 +6885,7 @@ static void rbd_dev_image_release(struct rbd_device *rbd_dev)
   * with @depth == 0.
   */
  static int rbd_dev_image_probe(struct rbd_device *rbd_dev, int depth)
+	__context_unsafe(conditional locking on @depth)
  {
  	bool need_watch = !rbd_is_ro(rbd_dev);
  	int ret;

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.

Quoted 12 lines · Nilay Shroff

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.

Quoted 2 lines

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

drivers/block/rbd.c +34 -8

index f0f7a94e3e3e..4c9f566b7a91 100644
@@ -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);

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.

Quoted 6 lines · Nilay Shroff

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.

Marco, to implement what Nilay is asking I need support for annotating function arguments with __guarded_by(). This would not only be useful for the rbd driver but also for other Linux kernel drivers, e.g. the loop driver. If I try to annotate function arguments in the loop driver with __guarded_by() the following error message appears:

drivers/block/loop.c:2002:30: warning: 'guarded_by' attribute only applies to non-static data members and global variables [-Wignored-attributes]
2002 | struct list_head *cmd_list
__guarded_by(&lo->lo_work_lock))

Your opinion about this is welcome.

Hi Jens,

This patch series enables lock context analysis in all block drivers in the drivers/block/ directory except DRBD. Please consider these patches for the next merge window.