fsync() on read-only RAID triggers BUG

We've had a report of the BUG in md_write_start() being triggered while running the Debian installer: <http://bugs.debian.org/696650>.
Based on the call trace, I came up with the following script that reproduces this under Linux 3.2.35 and 3.7.3:

--- BEGIN ---
#!/bin/bash -ex

# Set up temporary RAID1 on /dev/md0
dd if=/dev/zero of=/tmp/disk0 bs=1M count=1
losetup /dev/loop0 /tmp/disk0
dd if=/dev/zero of=/tmp/disk1 bs=1M count=1
losetup /dev/loop1 /tmp/disk1
yes | mdadm -C /dev/md0 -l 1 -n 2 /dev/loop{0,1}

# Make it read-only
while ! mdadm -o /dev/md0; do sleep 1; done

# Call fsync()
python -c "import os; os.fsync(os.open('/dev/md0', os.O_RDWR))" --- END ---

I assume that the sync request should be filtered out at some point before this assertion is made, since there can be nothing to sync.

Ben.

Ben Hutchings
Never attribute to conspiracy what can adequately be explained by stupidity.

I wrote a test case in C. It gets SIGSEGV upon fsync. When making the rdevs below also read-only the MD device can't be stopped anymore as it thinks that there is still active IO.

The attached patch should fix it. Please confirm. We have to return a completion without incrementing the active IO count. Error code -EROFS seems to be suited best.

But the libc fsync gets -EIO anyway:
Input/output error

Any objection?

Cheers,
Sebastian

From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH] md: protect against crash upon fsync on ro array

If an fsync occurrs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

As return value -EROFS makes most sense.

---
 drivers/md/md.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

index 3db3d1b..475e0be 100644
@@ -322,6 +322,11 @@ static void md_make_request(struct request_queue *q, struct bio *bio)
 		}
 		finish_wait(&mddev->sb_wait, &__wait);
 	}
+	if (mddev->ro == 1 && unlikely(rw == WRITE)) {
+		rcu_read_unlock();
+		bio_endio(bio, -EROFS);
+		return;
+	}
 	atomic_inc(&mddev->active_io);
 	rcu_read_unlock();
 

1.7.1

I applied this on top of 3.2.37 and it certainly fixes the crash. However I wonder whether fsync() should fail or should immediately succeed. I don't know whether the installer expects it to succeed.

Ben.

Ben Hutchings
Any smoothly functioning technology is indistinguishable from a rigged demo.

It should succeed.

Subject was: Re: fsync() on read-only RAID triggers BUG

O.K., then I hope Neil applies the attached patch. I've changed the return value to success.

This is also something for linux-stable and should apply to many kernel versions without an issue.

From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v2] md: protect against crash upon fsync on ro array

If an fsync occurrs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

By advice of Christoph Hellwig we silently return success.

---
 drivers/md/md.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

index 3db3d1b..475e0be 100644
@@ -322,6 +322,11 @@ static void md_make_request(struct request_queue *q, struct bio *bio)
 		}
 		finish_wait(&mddev->sb_wait, &__wait);
 	}
+	if (mddev->ro == 1 && unlikely(rw == WRITE)) {
+		rcu_read_unlock();
+		bio_endio(bio, 0);
+		return;
+	}
 	atomic_inc(&mddev->active_io);
 	rcu_read_unlock();
 

1.7.1

I've tried to race with continuous fsyncs against continuous "mdadm -o /dev/md0; mdadm -w /dev/md0;" in parallel but couldn't break it. Therefore, no additional locking is required and this part can be put directly before the RCU locking stuff and the suspended handling.

I've attached version 3 of the patch.

From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v3] md: protect against crash upon fsync on ro array

If an fsync occurrs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

By advice of Christoph Hellwig we silently return success.

---
 drivers/md/md.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

index 3db3d1b..6ba20f7 100644
@@ -307,6 +307,10 @@ static void md_make_request(struct request_queue *q, struct bio *bio)
 		bio_io_error(bio);
 		return;
 	}
+	if (mddev->ro == 1 && unlikely(rw == WRITE)) {
+		bio_endio(bio, 0);
+		return;
+	}
 	smp_rmb(); /* Ensure implications of  'active' are visible */
 	rcu_read_lock();
 	if (mddev->suspended) {

I'm slightly uneasy about returning 0 for all writes to a read-only deivce, because if someone ever fails to check the read-only flag elsewhere this could turn into silent data loss. Does this work:

		BUG_ON(bio_segments(bio));

Or should it be:

		bio_endio(bio, bio_segments(bio) == 0 ? 0 : -EROFS);

to make the error survivable?

Ben.

Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.

Good point. But it's better to use "bio_sectors" as bi_size is the important information. I've seen that DRBD detects empty flushes the same way.

For testing I've disabled the "set_disk_ro" in "md_set_readonly". Then, I did direct IO writes on the read-only array and it worked - I've received "Input/output error" in the user space.

I've attached version 4 of the patch.

Any further objection?

From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v4] md: protect against crash upon fsync on ro array

If an fsync occurrs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

By advice of Christoph Hellwig we return success upon a flush request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.

---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index 3db3d1b..1e634a6 100644
@@ -307,6 +307,10 @@ static void md_make_request(struct request_queue *q, struct bio *bio)
 		bio_io_error(bio);
 		return;
 	}
+	if (mddev->ro == 1 && unlikely(rw == WRITE)) {
+		bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+		return;
+	}
 	smp_rmb(); /* Ensure implications of  'active' are visible */
 	rcu_read_lock();
 	if (mddev->suspended) {

1.7.1

Am Dienstag, den 29.01.2013, 12:19 +0100 schrieb Sebastian Riemer:

[…]

Small typo (occurs) in commit message.

Thanks,

Paul

From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v5] md: protect against crash upon fsync on ro array

If an fsync occurs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

By advice of Christoph Hellwig we return success upon a flush request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.

---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index 3db3d1b..1e634a6 100644
@@ -307,6 +307,10 @@ static void md_make_request(struct request_queue *q, struct bio *bio)
 		bio_io_error(bio);
 		return;
 	}
+	if (mddev->ro == 1 && unlikely(rw == WRITE)) {
+		bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+		return;
+	}
 	smp_rmb(); /* Ensure implications of  'active' are visible */
 	rcu_read_lock();
 	if (mddev->suspended) {

1.7.1

Hi Neil,

please apply this patch! It is correct, now.

It applies to 3.2.y, 3.4.y, 3.7.y and latest 3.8-rc5. All these versions are affected by this bug.

3.0.y and 2.6.34.y are also affected. Please also find the patches for these versions attached. I've tested them. They work.

The strange thing is that 2.6.32.y is immune against this bug. So it must be a regression. The patch restores the same behavior as present in 2.6.32: fsync receives success.

I've tested against the following versions: 3.8-rc5, 3.7.5, 3.4.28, 3.2.37, 3.0.61, 2.6.34.14 and 2.6.32.60.

Cheers,
Sebastian

From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH v5] md: protect against crash upon fsync on ro array

If an fsync occurs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

By advice of Christoph Hellwig we return success upon a flush request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.

---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index 3db3d1b..1e634a6 100644
@@ -307,6 +307,10 @@ static void md_make_request(struct request_queue *q, struct bio *bio)
 		bio_io_error(bio);
 		return;
 	}
+	if (mddev->ro == 1 && unlikely(rw == WRITE)) {
+		bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+		return;
+	}
 	smp_rmb(); /* Ensure implications of  'active' are visible */
 	rcu_read_lock();
 	if (mddev->suspended) {

1.7.1

From 58ecc54ef2ea1692b2a608183901708d3cad5120 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH 3.0.y] md: protect against crash upon fsync on ro array
Reply-To: linux-raid <[email protected]>

If an fsync occurs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

By advice of Christoph Hellwig we return success upon a flush request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.

---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index 98262e5..4ef75e9 100644
@@ -299,6 +299,10 @@ static int md_make_request(struct request_queue *q, struct bio *bio)
 		bio_io_error(bio);
 		return 0;
 	}
+	if (mddev->ro == 1 && unlikely(rw == WRITE)) {
+		bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+		return 0;
+	}
 	smp_rmb(); /* Ensure implications of  'active' are visible */
 	rcu_read_lock();
 	if (mddev->suspended) {

1.7.1

From 58ecc54ef2ea1692b2a608183901708d3cad5120 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <[email protected]>
Date: Fri, 25 Jan 2013 12:46:59 +0100
Subject: [PATCH 2.6.34.y] md: protect against crash upon fsync on ro array
Reply-To: linux-raid <[email protected]>

If an fsync occurs on a read-only array, we need to send a completion for the IO and may not increment the active IO count. Otherwise, we hit a bug trace and can't stop the MD array anymore.

By advice of Christoph Hellwig we return success upon a flush request but we return -EROFS for other writes.
We detect flush requests by checking if the bio has zero sectors.

---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index d8e5adc..ba1c0be 100644
@@ -221,6 +221,10 @@ static int md_make_request(struct request_queue *q, struct bio *bio)
 		bio_io_error(bio);
 		return 0;
 	}
+	if (mddev->ro == 1 && unlikely(bio_data_dir(bio) == WRITE)) {
+		bio_endio(bio, bio_sectors(bio) == 0 ? 0 : -EROFS);
+		return 0;
+	}
 	rcu_read_lock();
 	if (mddev->suspended || mddev->barrier) {
 		DEFINE_WAIT(__wait);

1.7.1

Thanks!
I've added "Cc: [email protected]" and will forward it to Linus shortly.

NeilBrown