[PATCH] mmc: prevent dangling block device from accessing stale queues

[PATCH] mmc: prevent dangling block device from accessing stale queues

[Stefan Bader]

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

Kernel: 2.6.30-rc7 based
Worked in 2.6.28 (probably only because things went at a different speed)

Testcase: Use ext3/ext4 on a SD card partitioned with one primary DOS partition 
and leave it mounted while suspend/resume.

Result: After resume the partition table of the SD card has been erased.

The detailed description can be found at:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/383668

In essence the mmc block device frees the generic request queue before the last 
user of the gendisk has stopped using it leaving an invalid queue pointer which 
get unfortunately re-used before more requests come in for the old device.

The bugfix will cause more I/O error messages and might not be the ultimate way 
things should work, but it prevents data from getting lost.

Stefan


[-- Attachment #2: 0001-UBUNTU-Upstream-mmc-prevent-dangling-block-device-fr.patch --]
[-- Type: text/x-diff, Size: 2517 bytes --]

>From 3f8fa799dea815654381af2b12b0983e440c6c6e Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Wed, 3 Jun 2009 18:17:31 +0000
Subject: [PATCH] UBUNTU: [Upstream] mmc: prevent dangling block device from accessing stale queues

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/383668

When the mmc subsytem removes the block device (e.g. for suspend), it will
call mmc_cleanup_queue to release the block device request queue.
However the gendisk struct still has a pointer to that queue which is not
accounted for. If the block device is still open, the gendisk struct will
not get freed and might still use the stale pointer.
This gets even worse for the fact that (in this case) on resume, a new block
device is created which gets the same request queue object from the cache.
Now any stray access to that old block device end up on the new one.
As the functions to get and put the blk queue are not exported, the fix will
delay the actual call to blk_cleanup_queue until the last user of the mmc
block device drops its reference. Until then the blk queue is present but
will reject any I/O.

Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 drivers/mmc/card/block.c |    6 ++++++
 drivers/mmc/card/queue.c |    7 ++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 014b271..69d7cec 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -88,6 +88,12 @@ static void mmc_blk_put(struct mmc_blk_data *md)
 		int devidx = MINOR(disk_devt(md->disk)) >> MMC_SHIFT;
 		__clear_bit(devidx, dev_use);
 
+		/*
+		 * We are about to drop the last reference to the disk object.
+		 * Nothing else should now be looking at the queue pointer, so
+		 * now it won't hurt if we release it.
+		 */
+		blk_cleanup_queue(md->disk->queue);
 		put_disk(md->disk);
 		kfree(md);
 	}
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 4978562..163cc28 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -256,7 +256,12 @@ void mmc_cleanup_queue(struct mmc_queue *mq)
 		kfree(mq->bounce_buf);
 	mq->bounce_buf = NULL;
 
-	blk_cleanup_queue(mq->queue);
+	/*
+	 * Calling blk_cleanup_queue() would be too soon here. As long as
+	 * the gendisk has a reference to it and is not released we should
+	 * keep the queue. It has been shutdown and will not accept any new
+	 * requests, so that should be safe.
+	 */
 
 	mq->card = NULL;
 }
-- 
1.6.3.1

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Pierre Ossman]

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

On Thu, 04 Jun 2009 20:00:52 +0200
Stefan Bader <stefan.bader@canonical.com> wrote:

> Kernel: 2.6.30-rc7 based
> Worked in 2.6.28 (probably only because things went at a different speed)
> 
> Testcase: Use ext3/ext4 on a SD card partitioned with one primary DOS partition 
> and leave it mounted while suspend/resume.
> 
> Result: After resume the partition table of the SD card has been erased.
> 
> The detailed description can be found at:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/383668
> 
> In essence the mmc block device frees the generic request queue before the last 
> user of the gendisk has stopped using it leaving an invalid queue pointer which 
> get unfortunately re-used before more requests come in for the old device.
> 
> The bugfix will cause more I/O error messages and might not be the ultimate way 
> things should work, but it prevents data from getting lost.
> 

You seem to have dug a bit further than I've had time for. Do you have
anything substantial to back this up:

> +	/*
> +	 * Calling blk_cleanup_queue() would be too soon here. As long as
> +	 * the gendisk has a reference to it and is not released we should
> +	 * keep the queue. It has been shutdown and will not accept any new
> +	 * requests, so that should be safe.
> +	 */

?

It would seem that gendisk is making some bad assumptions and needs to
be changed if that is the case.

This part from the launchpad report also seems incredibly broken:

> What makes the whole thing a disaster is the fact that the block device queue objects are taken from a slub cache. Which means on resume, the newly created block device will get the same queue object as the old one, initializes it and
> after the tasks have been resumed, ext3 feels obliged to write out the invalidated superblocks (still not sure why it goes for sector 0) which will happily migrate to the new block device and cause confusion.

Jens, comments?

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

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

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Stefan Bader]

Pierre Ossman wrote:
> On Thu, 04 Jun 2009 20:00:52 +0200
> Stefan Bader <stefan.bader@canonical.com> wrote:
> 
>> Kernel: 2.6.30-rc7 based
>> Worked in 2.6.28 (probably only because things went at a different speed)
>>
>> Testcase: Use ext3/ext4 on a SD card partitioned with one primary DOS partition 
>> and leave it mounted while suspend/resume.
>>
>> Result: After resume the partition table of the SD card has been erased.
>>
>> The detailed description can be found at:
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/383668
>>
>> In essence the mmc block device frees the generic request queue before the last 
>> user of the gendisk has stopped using it leaving an invalid queue pointer which 
>> get unfortunately re-used before more requests come in for the old device.
>>
>> The bugfix will cause more I/O error messages and might not be the ultimate way 
>> things should work, but it prevents data from getting lost.
>>
> 
> You seem to have dug a bit further than I've had time for. Do you have
> anything substantial to back this up:
> 
>> +	/*
>> +	 * Calling blk_cleanup_queue() would be too soon here. As long as
>> +	 * the gendisk has a reference to it and is not released we should
>> +	 * keep the queue. It has been shutdown and will not accept any new
>> +	 * requests, so that should be safe.
>> +	 */
> 

This is mostly based on the debug output. But it seems hard to get around of it 
without having a way to increment the refcount of the queue. It is probably not 
the most common use case to remove a device while it is mounted.
Hm, not sure this is what you wanted to know... On the launchpad report there 
are logs which I took with lots of printk's enabled. This shows that after 
resume the queue receives a request from mmcblk0 (which no longer exists) but 
uses the same pointer as mmcblk1 which was just created.

> 
> It would seem that gendisk is making some bad assumptions and needs to
> be changed if that is the case.

I think the setup and release of it would need to have access to blk_queue_get 
and blk_queue_put. When it is created and the queue pointer is stored it should 
take a reference and when the object is finally released, reference to the 
queue would get dropped.

> This part from the launchpad report also seems incredibly broken:
> 
>> What makes the whole thing a disaster is the fact that the block device queue objects are taken from a slub cache. Which means on resume, the newly created block device will get the same queue object as the old one, initializes it and
>> after the tasks have been resumed, ext3 feels obliged to write out the invalidated superblocks (still not sure why it goes for sector 0) which will happily migrate to the new block device and cause confusion.

I don't think that part is that much broken. It is more a unfortunate result of 
the previous events. Maybe the part of ext3 writing to sector 0 is a bit 
worrying as I would only expect it to update the mount information which I hink 
is somewhere around sector 10.

> Jens, comments?
> 
> Rgds


-- 

When all other means of communication fail, try words!

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Matt Fleming]

On Thu, Jun 04, 2009 at 09:00:42PM +0200, Stefan Bader wrote:
>
> Hm, not sure this is what you wanted to know... On the launchpad report 
> there are logs which I took with lots of printk's enabled. This shows that 
> after resume the queue receives a request from mmcblk0 (which no longer 
> exists) but uses the same pointer as mmcblk1 which was just created.
>

Maybe I'm missing something, but why is the device instance being
destroyed during a suspend? E.g why do you have mmcblk0 before suspend and
mmcblk1 after suspend?

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Pierre Ossman]

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

On Thu, 04 Jun 2009 21:00:42 +0200
Stefan Bader <stefan.bader@canonical.com> wrote:

> Pierre Ossman wrote:
> > 
> > You seem to have dug a bit further than I've had time for. Do you have
> > anything substantial to back this up:
> > 
> >> +	/*
> >> +	 * Calling blk_cleanup_queue() would be too soon here. As long as
> >> +	 * the gendisk has a reference to it and is not released we should
> >> +	 * keep the queue. It has been shutdown and will not accept any new
> >> +	 * requests, so that should be safe.
> >> +	 */
> > 
> 
> This is mostly based on the debug output. But it seems hard to get around of it 
> without having a way to increment the refcount of the queue. It is probably not 
> the most common use case to remove a device while it is mounted.
> Hm, not sure this is what you wanted to know... On the launchpad report there 
> are logs which I took with lots of printk's enabled. This shows that after 
> resume the queue receives a request from mmcblk0 (which no longer exists) but 
> uses the same pointer as mmcblk1 which was just created.
> 

I was hoping you had dug around in the block layer and had some idea
why gendisk requires someone else to keep the queue around for it. Is
it just a simple case of a missing reference, or is there some
architectural problem?

> > This part from the launchpad report also seems incredibly broken:
> > 
> >> What makes the whole thing a disaster is the fact that the block device queue objects are taken from a slub cache. Which means on resume, the newly created block device will get the same queue object as the old one, initializes it and
> >> after the tasks have been resumed, ext3 feels obliged to write out the invalidated superblocks (still not sure why it goes for sector 0) which will happily migrate to the new block device and cause confusion.
> 
> I don't think that part is that much broken. It is more a unfortunate result of 
> the previous events. Maybe the part of ext3 writing to sector 0 is a bit 
> worrying as I would only expect it to update the mount information which I hink 
> is somewhere around sector 10.
> 

The incredibly broken part is how requests for the old queue wind up on
the new queue. Such a thing should never be possible.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

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

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Pierre Ossman]

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

On Thu, 4 Jun 2009 20:15:13 +0100
Matt Fleming <matt@console-pimps.org> wrote:

> 
> Maybe I'm missing something, but why is the device instance being
> destroyed during a suspend? E.g why do you have mmcblk0 before suspend and
> mmcblk1 after suspend?

Because the card gets powered down during suspend and we have no way of
detecting what has happened to it when we come back up. USB does the
same thing (although it has slightly more intelligent hardware which
can keep track of removal as long as the host has some power).

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

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

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Stefan Bader]

Matt Fleming wrote:
> On Thu, Jun 04, 2009 at 09:00:42PM +0200, Stefan Bader wrote:
>> Hm, not sure this is what you wanted to know... On the launchpad report 
>> there are logs which I took with lots of printk's enabled. This shows that 
>> after resume the queue receives a request from mmcblk0 (which no longer 
>> exists) but uses the same pointer as mmcblk1 which was just created.
>>
> 
> Maybe I'm missing something, but why is the device instance being
> destroyed during a suspend? E.g why do you have mmcblk0 before suspend and
> mmcblk1 after suspend?

That is the way mmcblock works  (without unsafe resume set) in conjunction with 
( probably ) slow userspace. On suspend the block device is removed. But the 
mount is cleaned by (in that case hald) doing a forced unmount. The timeing 
seems to be that the unmount part is partially done on the way up.

-- 

When all other means of communication fail, try words!

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Stefan Bader]

Pierre Ossman wrote:
> On Thu, 04 Jun 2009 21:00:42 +0200
> Stefan Bader <stefan.bader@canonical.com> wrote:
> 
>> Pierre Ossman wrote:
>>> You seem to have dug a bit further than I've had time for. Do you have
>>> anything substantial to back this up:
>>>
>>>> +	/*
>>>> +	 * Calling blk_cleanup_queue() would be too soon here. As long as
>>>> +	 * the gendisk has a reference to it and is not released we should
>>>> +	 * keep the queue. It has been shutdown and will not accept any new
>>>> +	 * requests, so that should be safe.
>>>> +	 */
>> This is mostly based on the debug output. But it seems hard to get around of it 
>> without having a way to increment the refcount of the queue. It is probably not 
>> the most common use case to remove a device while it is mounted.
>> Hm, not sure this is what you wanted to know... On the launchpad report there 
>> are logs which I took with lots of printk's enabled. This shows that after 
>> resume the queue receives a request from mmcblk0 (which no longer exists) but 
>> uses the same pointer as mmcblk1 which was just created.
>>
> 
> I was hoping you had dug around in the block layer and had some idea
> why gendisk requires someone else to keep the queue around for it. Is
> it just a simple case of a missing reference, or is there some
> architectural problem?
> 

You could say architectural. The get a queue object and the pointer to that 
gets stored in the gendisk object. This is used in generic make request to get 
the queue for a bdev. The reference to the bdev (this is a bit guessing) is 
kept by the filesystem.
The mmc block device will release the disk reference not before the last user 
is gone (again the fs). Another approach would have been to set the queue 
pointer to NULL after the queue has been released. But there is no locking 
around getting the pointer, so that seemed dangerous as well.

>>> This part from the launchpad report also seems incredibly broken:
>>>
>>>> What makes the whole thing a disaster is the fact that the block device queue objects are taken from a slub cache. Which means on resume, the newly created block device will get the same queue object as the old one, initializes it and
>>>> after the tasks have been resumed, ext3 feels obliged to write out the invalidated superblocks (still not sure why it goes for sector 0) which will happily migrate to the new block device and cause confusion.
>> I don't think that part is that much broken. It is more a unfortunate result of 
>> the previous events. Maybe the part of ext3 writing to sector 0 is a bit 
>> worrying as I would only expect it to update the mount information which I hink 
>> is somewhere around sector 10.
>>
> 
> The incredibly broken part is how requests for the old queue wind up on
> the new queue. Such a thing should never be possible.
>

That is only possible as the queue object s created from a cache. The old queue 
has been released and the new on re-uses that storage. This would be ok, but 
now pointer in the old gendisk is in fact crosspointing.

I think (but I have not debugged much into that direction) that I saw bad 
pointer dereferences on just ejecting the mounted sd card. Which probably was 
caused by the same issue. Just in that case the pointer is invalid and no new 
device has been created to be hit.


> Rgds


-- 

When all other means of communication fail, try words!

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Pavel Machek]

On Thu 2009-06-04 20:00:52, Stefan Bader wrote:
> Kernel: 2.6.30-rc7 based
> Worked in 2.6.28 (probably only because things went at a different speed)
>
> Testcase: Use ext3/ext4 on a SD card partitioned with one primary DOS 
> partition and leave it mounted while suspend/resume.
>
> Result: After resume the partition table of the SD card has been erased.
>
> The detailed description can be found at:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/383668
>
> In essence the mmc block device frees the generic request queue before 
> the last user of the gendisk has stopped using it leaving an invalid 
> queue pointer which get unfortunately re-used before more requests come 
> in for the old device.
>
> The bugfix will cause more I/O error messages and might not be the 
> ultimate way things should work, but it prevents data from getting lost.

Thanks for finding root cause of this!
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Stefan Bader]

I did not see any news related to this issue. Pierre, are you waiting
on more info from my side? Or did I miss a change somewhere else?

Stefan


> On Thu 2009-06-04 20:00:52, Stefan Bader wrote:
>> Kernel: 2.6.30-rc7 based
>> Worked in 2.6.28 (probably only because things went at a different speed)
>>
>> Testcase: Use ext3/ext4 on a SD card partitioned with one primary DOS
>> partition and leave it mounted while suspend/resume.
>>
>> Result: After resume the partition table of the SD card has been erased.
>>
>> The detailed description can be found at:
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/383668
>>
>> In essence the mmc block device frees the generic request queue before
>> the last user of the gendisk has stopped using it leaving an invalid
>> queue pointer which get unfortunately re-used before more requests come
>> in for the old device.
>>
>> The bugfix will cause more I/O error messages and might not be the
>> ultimate way things should work, but it prevents data from getting lost.
>

Re: [PATCH] mmc: prevent dangling block device from accessing stale queues

[Pierre Ossman]

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

On Tue, 23 Jun 2009 17:01:14 +0200
Stefan Bader <stefan.bader@canonical.com> wrote:

> I did not see any news related to this issue. Pierre, are you waiting
> on more info from my side? Or did I miss a change somewhere else?
> 

I plan to have a closer look at this first. I believe your solution is
a workaround and doesn't solve the real issue. As such, I want to have
one more go at finding and fixing the real problem before committing
this. Your analysis should help a lot in that effort.

Rgds
-- 
     -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

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