On 08/15/2017 07:11 PM, Steven Rostedt wrote:

On Thu, 10 Aug 2017 13:02:33 -0400
Waiman Long [off-list ref] wrote:

quoted

The lockdep code had reported the following unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(s_active#228);
                               lock(&bdev->bd_mutex/1);
                               lock(s_active#228);
  lock(&bdev->bd_mutex);

 *** DEADLOCK ***

The deadlock may happen when one task (CPU1) is trying to delete
a partition in a block device and another task (CPU0) is accessing
tracing sysfs file in that partition.

To avoid that, accessing tracing sysfs file will now use a mutex
trylock loop and the operation will fail if a delete operation is
in progress.

This is wrong at a lot of levels.

quoted

Signed-off-by: Waiman Long <longman@redhat.com>
---
 block/ioctl.c           |  3 +++
 include/linux/fs.h      |  1 +
 kernel/trace/blktrace.c | 24 ++++++++++++++++++++++--
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 0de02ee..7211608 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c

@@ -86,12 +86,15 @@ static int blkpg_ioctl(struct block_device *bdev, =

struct blkpg_ioctl_arg __user

quoted

 				return -EBUSY;
 			}
 			/* all seems OK */
+			bdev->bd_deleting =3D 1;

Note, one would need a  memory barrier here. But I'm not sure how much
of a fast path this location is.

			/*
			 * Make sure bd_deleting is set before taking
			 * mutex.
			 */
			smp_mb();

You are right. Some sort of memory barrier is needed to make sure that
the setting of bd_deleting won't get reordered into the
mutex_lock/mutex_unlock critical section.

quoted

 			fsync_bdev(bdevp);
 			invalidate_bdev(bdevp);
=20
 			mutex_lock_nested(&bdev->bd_mutex, 1);
 			delete_partition(disk, partno);
 			mutex_unlock(&bdev->bd_mutex);
+			bdev->bd_deleting =3D 0;
+
 			mutex_unlock(&bdevp->bd_mutex);
 			bdput(bdevp);
=20

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7b5d681..5d4ae9d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h

@@ -427,6 +427,7 @@ struct block_device {
 #endif
 	struct block_device *	bd_contains;
 	unsigned		bd_block_size;
+	int			bd_deleting;
 	struct hd_struct *	bd_part;
 	/* number of times partitions within this device have been opened. *=

/

quoted

 	unsigned		bd_part_count;

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index bc364f8..715e77e 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c

@@ -1605,6 +1605,18 @@ static struct request_queue *blk_trace_get_queu=

e(struct block_device *bdev)

quoted

 	return bdev_get_queue(bdev);
 }
=20
+/*
+ * Read/write to the tracing sysfs file requires taking references to=

 the

quoted

+ * sysfs file and then acquiring the bd_mutex. Deleting a block devic=

e

quoted

+ * requires acquiring the bd_mutex and then waiting for all the sysfs=

quoted

+ * references to be gone. This can lead to deadlock if both operation=

s

quoted

+ * happen simultaneously. To avoid this problem, read/write to the
+ * the tracing sysfs files can now fail if the bd_mutex cannot be
+ * acquired while a deletion operation is in progress.
+ *
+ * A mutex trylock loop is used assuming that tracing sysfs operation=

s

quoted

+ * aren't frequently enough to cause any contention problem.
+ */
 static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)

@@ -1622,7 +1634,11 @@ static ssize_t sysfs_blk_trace_attr_show(struct=

 device *dev,

quoted

 	if (q =3D=3D NULL)
 		goto out_bdput;
=20
-	mutex_lock(&bdev->bd_mutex);
+	while (!mutex_trylock(&bdev->bd_mutex)) {

		/* Make sure trylock happens before reading bd_deleting */
		smp_mb();

Since we don't take the lock, there's nothing that prevents the CPU
from fetching bd_deleting early, and this going into an infinite spin
even though bd_deleting is clear (without the memory barriers).

quoted

+		if (bdev->bd_deleting)
+			goto out_bdput;

We don't need a memory barrier here. Just a READ_ONCE() to make sure
that the compiler won't optimize the read out of the mutex_trylock()
loop is enough.

You also just turned the mutex into a spinlock. What happens if we just=

preempted the owner of bdev->bd_mutex and are an RT task with higher
priority? This will turn into a live lock.

quoted

+		schedule();
+	}
=20

That is OK because I used schedule() instead of cpu_relax() for
inserting delay.

Thanks for the comment. I will send out an updated patch later today.

Cheers,
Longman