linux-kernel - Re: How to cleanly shut down a block device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20061123210322.GB25794@flint.arm.linux.org.uk>
Date:	Thu, 23 Nov 2006 21:03:22 +0000
From:	Russell King <rmk+lkml@....linux.org.uk>
To:	Pierre Ossman <drzeus-list@...eus.cx>
Cc:	Jens Axboe <jens.axboe@...cle.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: How to cleanly shut down a block device

On Tue, Nov 14, 2006 at 03:34:25PM +0100, Pierre Ossman wrote:
> How about this patch? It is basically the same as the previous, but it
> also sets queuedata to NULL and tests for that. It does not address if
> someone still has dependencies on the queue but hasn't gotten itself a
> reference (as I haven't gotten any word on if that is a problem):
> 
> diff --git a/drivers/mmc/mmc_block.c b/drivers/mmc/mmc_block.c
> index f9027c8..5025abe 100644
> --- a/drivers/mmc/mmc_block.c
> +++ b/drivers/mmc/mmc_block.c
> @@ -83,7 +83,6 @@ static void mmc_blk_put(struct mmc_blk_d
>         md->usage--;
>         if (md->usage == 0) {
>                 put_disk(md->disk);
> -               mmc_cleanup_queue(&md->queue);
>                 kfree(md);
>         }
>         mutex_unlock(&open_lock);
> @@ -553,12 +552,11 @@ static void mmc_blk_remove(struct mmc_ca
>         if (md) {
>                 int devidx;
>  
> +               /* Stop new requests from getting into the queue */
>                 del_gendisk(md->disk);

I'm not entirely convinced that del_gendisk() will prevent any new
requests getting into the queue, which is why the comment below
says:

>  
> -               /*
> -                * I think this is needed.
> -                */
> -               md->disk->queue = NULL;

Yes, del_gendisk() removes the disk from view for new opens, but what
about existing users?

As the code currently stands, when I remove a mounted MMC card, I get:

generic_make_request: Trying to access nonexistent block-device mmcblk0p1 (26)
FAT: Directory bread(block 26) failed
generic_make_request: Trying to access nonexistent block-device mmcblk0p1 (27)
FAT: Directory bread(block 27) failed
... etc ...

and this comes from:

                q = bdev_get_queue(bio->bi_bdev);
                if (!q) {
                        printk(KERN_ERR ...

in block/ll_rw_blk.c.  bdev_get_queue merely does:

        return bdev->bd_disk->queue;

> +               /* Then flush out any already in there */
> +               mmc_cleanup_queue(&md->queue);

Now, the thing is calling mmc_cleanup_queue() will shutdown the thread
and wait for it to complete its business, free the scatterlist, and
call blk_cleanup_queue() on that queue.

Ergo, once we've shut down the thread, we should not get any further
MMC requests, so:

> diff --git a/drivers/mmc/mmc_queue.c b/drivers/mmc/mmc_queue.c
> index 4ccdd82..b6769e2 100644
> --- a/drivers/mmc/mmc_queue.c
> +++ b/drivers/mmc/mmc_queue.c
> @@ -111,6 +111,19 @@ static int mmc_queue_thread(void *d)
>  static void mmc_request(request_queue_t *q)
>  {
>         struct mmc_queue *mq = q->queuedata;
> +       struct request *req;
> +       int ret;
> +
> +       if (!mq) {
> +               printk(KERN_ERR "MMC: killing requests for dead queue\n");
> +               while ((req = elv_next_request(q)) != NULL) {
> +                       do {
> +                               ret = end_that_request_chunk(req, 0,
> +                                       req->current_nr_sectors << 9);
> +                       } while (ret);
> +               }
> +               return;
> +       }
>  
>         if (!mq->req)
>                 wake_up(&mq->thread_wq);
> @@ -179,6 +192,15 @@ EXPORT_SYMBOL(mmc_init_queue);
>  
>  void mmc_cleanup_queue(struct mmc_queue *mq)
>  {
> +       request_queue_t *q = mq->queue;
> +       unsigned long flags;
> +
> +       /* Mark that we should start throwing out stragglers */
> +       spin_lock_irqsave(q->queue_lock, flags);
> +       q->queuedata = NULL;
> +       spin_unlock_irqrestore(q->queue_lock, flags);
> +
> +       /* Then terminate our worker thread */
>         mq->flags |= MMC_QUEUE_EXIT;
>         wake_up(&mq->thread_wq);
>         wait_for_completion(&mq->thread_complete);

These hunks do not achieve us anything.

However, what we _do_ need to do is to arrange for the MMC queue thread
to error out all pending requests before dying if MMC_QUEUE_EXIT is set.
That's already handled since the queue thread only ever exits if there
are no requests pending _and_ MMC_QUEUE_EXIT has been set.

So I think all you really need is this change:

diff --git a/drivers/mmc/mmc_block.c b/drivers/mmc/mmc_block.c
index f9027c8..9173067 100644
--- a/drivers/mmc/mmc_block.c
+++ b/drivers/mmc/mmc_block.c
@@ -83,7 +83,6 @@ static void mmc_blk_put(struct mmc_blk_d
 	md->usage--;
 	if (md->usage == 0) {
 		put_disk(md->disk);
-		mmc_cleanup_queue(&md->queue);
 		kfree(md);
 	}
 	mutex_unlock(&open_lock);
@@ -559,6 +558,7 @@ static void mmc_blk_remove(struct mmc_ca
 		 * I think this is needed.
 		 */
 		md->disk->queue = NULL;
+		mmc_cleanup_queue(&md->queue);
 
 		devidx = md->disk->first_minor >> MMC_SHIFT;
 		__clear_bit(devidx, dev_use);


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/