Re: [PATCH] drivers: mmc: reordered shutdown sequence in mmc_bld_remove_req

[Paul Taysom]

On Mon, Jun 10, 2013 at 2:44 AM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> On 4 June 2013 23:42, Paul Taysom <taysom@xxxxxxxxxxxx> wrote:
>> We had a multi-partition SD-Card with two ext2 file systems. The partition
>> table was getting overwritten by a race between the card removal and
>> the unmount of the 2nd ext2 partition.
>>
>> What was observed:
>> 1. Suspend/resume would call to remove the device. The clearing
>>    of the device information is done asynchronously.
>> 2. A request is made to unmount the file system (this is called
>>    after the removal has started).
>> 3. The remapping table was cleared by the asynchronous part of
>>    the device removal.
>> 4. A write request to the super block (block 0 of the partition)
>>    was sent down and instead of being remapped to the partition
>>    offset, it was remapped to block 0 of the device which is where
>>    the partition table is located.
>> 5. Write was queued and written resulting in the overwriting
>>    of the partition table with the ext2 super block.
>> 6. The mmc_queue is cleaned up.
>
> Hi Paul,
>
> An interesting bug you found here. My impression is that this is
> something that should be addressed through the blk layer, somehow.
>
> Have you considered that this is not only a problem for SD cards, but
> for other block device drivers as well. I believe it is common to call
> del_gendisk before blk_cleanup_queue, which in principle is what you
> want to change.
>
>>
>> The mmc card device driver used to access SD cards, was calling del_gendisk
>> before calling mmc_cleanup-queue. The comment in the mmc_blk_remove_req
>> code indicated that it expected del_gendisk to block all further requests
>> from being queued but it doesn't. The mmc driver uses the presences of the
>> mmc_queue to determine if the request should be queued.
>>
>> The fix was to clean up the mmc_queue before the rest of the
>> the delete partition code is called.
>>
>> This prevents the overwriting of the partition table.
>>
>> However, the umount gets an error trying to write the super block.
>> The umount should be issued before the device is removed but that
>> is not always possible. The umount is still needed to cleanup other
>> data structures.
>
> So this clearly indicates to me that this is not the complete
> solution, even if it solves the most serious problem for this bug.
>
> I think it would be good to get a blk device maintainer's input to
> this discussion.
>
> Kind regards
> Ulf Hansson
>
>>
>> Addresses the problem described in http://crbug.com/240815
>>
>> Signed-off-by: Paul Taysom <taysom@xxxxxxxxxxxx>
>> ---
>>  drivers/mmc/card/block.c | 14 ++++++++------
>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index dd27b07..a79f113 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -2158,6 +2158,14 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
>>         struct mmc_card *card;
>>
>>         if (md) {
>> +               /*
>> +                * Flush remaining requests and free queues. It
>> +                * is freeing the queue that stops new requests
>> +                * from being accepted.
>> +                */
>> +               mmc_cleanup_queue(&md->queue);
>> +               if (md->flags & MMC_BLK_PACKED_CMD)
>> +                       mmc_packed_clean(&md->queue);
>>                 card = md->queue.card;
>>                 if (md->disk->flags & GENHD_FL_UP) {
>>                         device_remove_file(disk_to_dev(md->disk), &md->force_ro);
>> @@ -2166,14 +2174,8 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
>>                                 device_remove_file(disk_to_dev(md->disk),
>>                                         &md->power_ro_lock);
>>
>> -                       /* Stop new requests from getting into the queue */
>>                         del_gendisk(md->disk);
>>                 }
>> -
>> -               /* Then flush out any already in there */
>> -               mmc_cleanup_queue(&md->queue);
>> -               if (md->flags & MMC_BLK_PACKED_CMD)
>> -                       mmc_packed_clean(&md->queue);
>>                 mmc_blk_put(md);
>>         }
>>  }
>> --
>> 1.8.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Added the block maintainer, axboe@xxxxxxxxx and others to CC list.

You are correct that this is probably not an isolated problem. I
looked at a number of drivers and they did follow the same pattern but
I have not yet looked at them deeply enough to see what they check
before queuing a request. For example, if they check part->nr_sects ==
0, they should work fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/