All of lore.kernel.org
 help / color / mirror / Atom feed
* fsync() on read-only RAID triggers BUG
@ 2013-01-20 18:44 Ben Hutchings
  2013-01-25 15:09 ` Sebastian Riemer
  0 siblings, 1 reply; 11+ messages in thread
From: Ben Hutchings @ 2013-01-20 18:44 UTC (permalink / raw)
  To: Neil Brown; +Cc: linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fsync() on read-only RAID triggers BUG
  2013-01-20 18:44 fsync() on read-only RAID triggers BUG Ben Hutchings
@ 2013-01-25 15:09 ` Sebastian Riemer
  2013-01-26 19:44   ` Ben Hutchings
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Riemer @ 2013-01-25 15:09 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Neil Brown, linux-raid

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

On 20.01.2013 19:44, Ben Hutchings wrote:
> # 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.
> 

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

[-- Attachment #2: 0001-md-protect-against-crash-upon-fsync-on-ro-array.txt --]
[-- Type: text/plain, Size: 1066 bytes --]

From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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.

Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
---
 drivers/md/md.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..475e0be 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: fsync() on read-only RAID triggers BUG
  2013-01-25 15:09 ` Sebastian Riemer
@ 2013-01-26 19:44   ` Ben Hutchings
  2013-01-27 16:39     ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Ben Hutchings @ 2013-01-26 19:44 UTC (permalink / raw)
  To: Sebastian Riemer; +Cc: Neil Brown, linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Fri, 2013-01-25 at 16:09 +0100, Sebastian Riemer wrote:
> On 20.01.2013 19:44, Ben Hutchings wrote:
> > # 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.
> > 
> 
> 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.

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.

> 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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fsync() on read-only RAID triggers BUG
  2013-01-26 19:44   ` Ben Hutchings
@ 2013-01-27 16:39     ` Christoph Hellwig
  2013-01-28 10:32       ` [PATCH v2] md: protect against crash upon fsync on ro array Sebastian Riemer
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2013-01-27 16:39 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Sebastian Riemer, Neil Brown, linux-raid, 696650

On Sat, Jan 26, 2013 at 07:44:40PM +0000, Ben Hutchings wrote:
> 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.

It should succeed.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] md: protect against crash upon fsync on ro array
  2013-01-27 16:39     ` Christoph Hellwig
@ 2013-01-28 10:32       ` Sebastian Riemer
  2013-01-28 12:39         ` [PATCH v3] " Sebastian Riemer
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Riemer @ 2013-01-28 10:32 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Ben Hutchings, Neil Brown, linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

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

On 27.01.2013 17:39, Christoph Hellwig wrote:
> On Sat, Jan 26, 2013 at 07:44:40PM +0000, Ben Hutchings wrote:
>> 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.
> 
> It should succeed.

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.

[-- Attachment #2: 0001-md-protect-against-crash-upon-fsync-on-ro-array.txt --]
[-- Type: text/plain, Size: 1244 bytes --]

From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>

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

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..475e0be 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3] md: protect against crash upon fsync on ro array
  2013-01-28 10:32       ` [PATCH v2] md: protect against crash upon fsync on ro array Sebastian Riemer
@ 2013-01-28 12:39         ` Sebastian Riemer
  2013-01-29  5:45           ` Ben Hutchings
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Riemer @ 2013-01-28 12:39 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Ben Hutchings, Neil Brown, linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

On 28.01.2013 11:32, Sebastian Riemer wrote:
> 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.
> 

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.

[-- Attachment #2: 0001-md-protect-against-crash-upon-fsync-on-ro-array.txt --]
[-- Type: text/plain, Size: 1250 bytes --]

From fe0357344877c9b9cc623fd582a4e0670e448317 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>

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

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..6ba20f7 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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) {

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] md: protect against crash upon fsync on ro array
  2013-01-28 12:39         ` [PATCH v3] " Sebastian Riemer
@ 2013-01-29  5:45           ` Ben Hutchings
  2013-01-29 11:19             ` [PATCH v4] " Sebastian Riemer
  0 siblings, 1 reply; 11+ messages in thread
From: Ben Hutchings @ 2013-01-29  5:45 UTC (permalink / raw)
  To: Sebastian Riemer; +Cc: Christoph Hellwig, Neil Brown, linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

On Mon, 2013-01-28 at 13:39 +0100, Sebastian Riemer wrote:
> 
> On 28.01.2013 11:32, Sebastian Riemer wrote:
> > 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.
> > 
> 
> 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 <sebastian.riemer@profitbricks.com>
> 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.
> 
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Ben Hutchings <ben@decadent.org.uk>
> Cc: NeilBrown <neilb@suse.de>
> Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
> Reported-by: Ben Hutchings <ben@decadent.org.uk>
> 
> ---
>  drivers/md/md.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 3db3d1b..6ba20f7 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v4] md: protect against crash upon fsync on ro array
  2013-01-29  5:45           ` Ben Hutchings
@ 2013-01-29 11:19             ` Sebastian Riemer
  2013-01-29 12:29               ` [PATCH v5] " Paul Menzel
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Riemer @ 2013-01-29 11:19 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Christoph Hellwig, Neil Brown, linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On 29.01.2013 06:45, Ben Hutchings wrote:
> 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?

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?

[-- Attachment #2: 0001-md-protect-against-crash-upon-fsync-on-ro-array.txt --]
[-- Type: text/plain, Size: 1408 bytes --]

From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..1e634a6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v5] md: protect against crash upon fsync on ro array
  2013-01-29 11:19             ` [PATCH v4] " Sebastian Riemer
@ 2013-01-29 12:29               ` Paul Menzel
  2013-01-31 19:35                 ` Sebastian Riemer
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Menzel @ 2013-01-29 12:29 UTC (permalink / raw)
  To: Sebastian Riemer
  Cc: Ben Hutchings, Christoph Hellwig, Neil Brown, linux-raid, 696650


[-- Attachment #1.1: Type: text/plain, Size: 170 bytes --]

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

[…]

> Any further objection?

Small typo (occurs) in commit message.


Thanks,

Paul


[-- Attachment #1.2: 0001-md-protect-against-crash-upon-fsync-on-ro-array.txt --]
[-- Type: text/plain, Size: 1504 bytes --]

From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..1e634a6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v5] md: protect against crash upon fsync on ro array
  2013-01-29 12:29               ` [PATCH v5] " Paul Menzel
@ 2013-01-31 19:35                 ` Sebastian Riemer
  2013-02-04 22:30                   ` NeilBrown
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Riemer @ 2013-01-31 19:35 UTC (permalink / raw)
  To: NeilBrown
  Cc: Ben Hutchings, Christoph Hellwig, Paul Menzel, linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

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


On 29.01.2013 13:29, Paul Menzel wrote:
>> Any further objection?
> 
> Small typo (occurs) in commit message.


[-- Attachment #2: 0001-md-protect-against-crash-upon-fsync-on-ro-array.txt --]
[-- Type: text/plain, Size: 1504 bytes --]

From adfac4df99edc1a83dced9c732464634d3381a9f Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 3db3d1b..1e634a6 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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

[-- Attachment #3: 0002-md-protect-against-crash-upon-fsync-on-ro-array-3.0.y.txt --]
[-- Type: text/plain, Size: 1521 bytes --]

From 58ecc54ef2ea1692b2a608183901708d3cad5120 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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 <linux-raid@vger.kernel.org>

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.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 98262e5..4ef75e9 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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


[-- Attachment #4: 0003-md-protect-against-crash-upon-fsync-on-ro-array-2.6.34.y.txt --]
[-- Type: text/plain, Size: 1516 bytes --]

From 58ecc54ef2ea1692b2a608183901708d3cad5120 Mon Sep 17 00:00:00 2001
From: Sebastian Riemer <sebastian.riemer@profitbricks.com>
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 <linux-raid@vger.kernel.org>

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.

Cc: Christoph Hellwig <hch@infradead.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Riemer <sebastian.riemer@profitbricks.com>
Reported-by: Ben Hutchings <ben@decadent.org.uk>
Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
---
 drivers/md/md.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d8e5adc..ba1c0be 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v5] md: protect against crash upon fsync on ro array
  2013-01-31 19:35                 ` Sebastian Riemer
@ 2013-02-04 22:30                   ` NeilBrown
  0 siblings, 0 replies; 11+ messages in thread
From: NeilBrown @ 2013-02-04 22:30 UTC (permalink / raw)
  To: Sebastian Riemer
  Cc: Ben Hutchings, Christoph Hellwig, Paul Menzel, linux-raid, 696650

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Thu, 31 Jan 2013 20:35:22 +0100 Sebastian Riemer
<sebastian.riemer@profitbricks.com> wrote:

> 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

Thanks!
I've added "Cc: stable@vger.kernel.org" and will forward it to Linus shortly.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-02-04 22:30 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-20 18:44 fsync() on read-only RAID triggers BUG Ben Hutchings
2013-01-25 15:09 ` Sebastian Riemer
2013-01-26 19:44   ` Ben Hutchings
2013-01-27 16:39     ` Christoph Hellwig
2013-01-28 10:32       ` [PATCH v2] md: protect against crash upon fsync on ro array Sebastian Riemer
2013-01-28 12:39         ` [PATCH v3] " Sebastian Riemer
2013-01-29  5:45           ` Ben Hutchings
2013-01-29 11:19             ` [PATCH v4] " Sebastian Riemer
2013-01-29 12:29               ` [PATCH v5] " Paul Menzel
2013-01-31 19:35                 ` Sebastian Riemer
2013-02-04 22:30                   ` NeilBrown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.