blk_cleanup_queue() versus request_fn order fix
Subsystems (3)
v3 cover blk_cleanup_queue() versus request_fn order fix
At device removal time request processing functions like scsi_request_fn() that unlock the queue lock internally may cause blk_cleanup_queue() to finish while request_fn is in progress. This three-patch series makes sure that blk_cleanup_queue() can't finish before all active request_fn calls have finished and also that request_fn isn't invoked anymore after queue draining finished.
Changes compared to v2:
- Split second patch into two patches.
- Refined patch descriptions.
Changes compared to v1:
- Included a patch to rename QUEUE_FLAG_DEAD.
- Refined the descriptions of the __blk_run_queue_uncond() and blk_cleanup_queue() functions.
PATCH v3 1/3 block: Rename queue dead flag
This flag is used to indicate that queuing new requests must stop. After this flag has been set queue draining starts. However, during the queue draining phase it is still safe to invoke the queue's request_fn, so "dying" is a better name than "dead".
This patch is the result of running the command below and manually fixing up indentation:
git grep -lE 'blk_queue_dead|QUEUE_FLAG_DEAD' |
xargs sed -i .tmp -e 's/blk_queue_dead/blk_queue_dying/g' \
-e 's/QUEUE_FLAG_DEAD/QUEUE_FLAG_DYING/g'
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Chanho Min <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- block/blk-cgroup.c | 2 +- block/blk-core.c | 16 ++++++++-------- block/blk-exec.c | 2 +- block/blk-sysfs.c | 4 ++-- block/blk-throttle.c | 2 +- block/blk.h | 2 +- drivers/block/ub.c | 2 +- drivers/scsi/scsi_lib.c | 2 +- include/linux/blkdev.h | 4 ++-- 9 files changed, 18 insertions(+), 18 deletions(-)
block/blk-cgroup.c +1 -1
@@ -231,7 +231,7 @@ struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,* we shouldn't allow anything to go through for a bypassing queue. */ if (unlikely (blk_queue_bypass (q))) -return ERR_PTR (blk_queue_dead (q) ? -EINVAL : -EBUSY ); +return ERR_PTR (blk_queue_dying (q) ? -EINVAL : -EBUSY );return __blkg_lookup_create (blkcg, q,NULL ); }EXPORT_SYMBOL_GPL (blkg_lookup_create);
block/blk-core.c +8 -8
@@ -484,7 +484,7 @@ void blk_cleanup_queue(struct request_queue *q)/* mark @q DEAD, no new request or merges will be allowed afterwards */ mutex_lock (&q->sysfs_lock); -queue_flag_set_unlocked (QUEUE_FLAG_DEAD , q); +queue_flag_set_unlocked (QUEUE_FLAG_DYING , q);spin_lock_irq (lock);/*
@@ -501,7 +501,7 @@ void blk_cleanup_queue(struct request_queue *q)queue_flag_set (QUEUE_FLAG_NOMERGES , q);queue_flag_set (QUEUE_FLAG_NOXMERGES , q); -queue_flag_set (QUEUE_FLAG_DEAD , q); +queue_flag_set (QUEUE_FLAG_DYING , q);spin_unlock_irq (lock);mutex_unlock (&q->sysfs_lock);
@@ -723,7 +723,7 @@ EXPORT_SYMBOL(blk_init_allocated_queue);bool blk_get_queue (struct request_queue *q) { -if (likely (!blk_queue_dead (q))) { +if (likely (!blk_queue_dying (q))) {__blk_get_queue (q);return true; }
@@ -877,7 +877,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,const bool is_sync =rw_is_sync (rw_flags) !=0 ;int may_queue; -if (unlikely (blk_queue_dead (q))) +if (unlikely (blk_queue_dying (q)))return NULL ; may_queue =elv_may_queue (q, rw_flags);
@@ -1057,7 +1057,7 @@ retry:if (rq)return rq; -if (!(gfp_mask & __GFP_WAIT) ||unlikely (blk_queue_dead (q))) { +if (!(gfp_mask & __GFP_WAIT) ||unlikely (blk_queue_dying (q))) {blk_put_rl (rl);return NULL ; }
@@ -1909,7 +1909,7 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq)return -EIO ;spin_lock_irqsave (q->queue_lock, flags); -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) {spin_unlock_irqrestore (q->queue_lock, flags);return -ENODEV ; }
@@ -2891,7 +2891,7 @@ static void queue_unplugged(struct request_queue *q, unsigned int depth,/* * Don't mess with dead queue. */ -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) {spin_unlock (q->queue_lock);return ; }
@@ -3000,7 +3000,7 @@ void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)/* * Short-circuit if @q is dead */ -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) {__blk_end_request_all (rq, -ENODEV );continue ; }
block/blk-exec.c +1 -1
@@ -60,7 +60,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,spin_lock_irq (q->queue_lock); -if (unlikely (blk_queue_dead (q))) { +if (unlikely (blk_queue_dying (q))) { rq->errors = -ENXIO ;if (rq->end_io) rq->end_io (rq, rq->errors);
block/blk-sysfs.c +2 -2
@@ -432,7 +432,7 @@ queue_attr_show(struct kobject *kobj, struct attribute *attr, char *page)if (!entry->show)return -EIO ;mutex_lock (&q->sysfs_lock); -if (blk_queue_dead (q)) { +if (blk_queue_dying (q)) {mutex_unlock (&q->sysfs_lock);return -ENOENT ; }
@@ -454,7 +454,7 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr, q = container_of(kobj, struct request_queue, kobj);mutex_lock (&q->sysfs_lock); -if (blk_queue_dead (q)) { +if (blk_queue_dying (q)) {mutex_unlock (&q->sysfs_lock);return -ENOENT ; }
block/blk-throttle.c +1 -1
@@ -303,7 +303,7 @@ static struct throtl_grp *throtl_lookup_create_tg(struct throtl_data *td,/* if %NULL and @q is alive, fall back to root_tg */ if (!IS_ERR (blkg)) tg =blkg_to_tg (blkg); -else if (!blk_queue_dead (q)) +else if (!blk_queue_dying (q)) tg =td_root_tg (td); }
block/blk.h +1 -1
@@ -96,7 +96,7 @@ static inline struct request *__elv_next_request(struct request_queue *q) q->flush_queue_delayed =1 ;return NULL ; } -if (unlikely (blk_queue_dead (q)) || +if (unlikely (blk_queue_dying (q)) || !q->elevator->type->ops.elevator_dispatch_fn (q,0 ))return NULL ; }
drivers/block/ub.c +1 -1
@@ -2394,7 +2394,7 @@ static void ub_disconnect(struct usb_interface *intf)del_gendisk (lun->disk);/* * I wish I could do: -* queue_flag_set(QUEUE_FLAG_DEAD, q); +* queue_flag_set(QUEUE_FLAG_DYING, q); * As it is, we rely on our internal poisoning and let * the upper levels to spin furiously failing all the I/O. */
drivers/scsi/scsi_lib.c +1 -1
@@ -1406,7 +1406,7 @@ static int scsi_lld_busy(struct request_queue *q)struct scsi_device *sdev = q->queuedata;struct Scsi_Host *shost; -if (blk_queue_dead (q)) +if (blk_queue_dying (q))return 0 ; shost = sdev->host;
include/linux/blkdev.h +2 -2
@@ -436,7 +436,7 @@ struct request_queue {#define QUEUE_FLAG_STOPPED 2/* queue is stopped */ #define QUEUE_FLAG_SYNCFULL 3/* read queue has been filled */ #define QUEUE_FLAG_ASYNCFULL 4/* write queue has been filled */ -#define QUEUE_FLAG_DEAD 5/* queue being torn down */ +#define QUEUE_FLAG_DYING 5/* queue being torn down */ #define QUEUE_FLAG_BYPASS 6/* act as dumb FIFO queue */ #define QUEUE_FLAG_BIDI 7/* queue supports bidi requests */ #define QUEUE_FLAG_NOMERGES 8/* disable merge attempts */
@@ -520,7 +520,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)#define blk_queue_tagged (q) test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)#define blk_queue_stopped (q) test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags) -#define blk_queue_dead (q) test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags) +#define blk_queue_dying (q) test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)#define blk_queue_bypass (q) test_bit(QUEUE_FLAG_BYPASS, &(q)->queue_flags)#define blk_queue_nomerges (q) test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)#define blk_queue_noxmerges (q) \
Signature · 1 line
1.7.10.4
PATCH v3 2/3 block: Avoid that request_fn is invoked on a dead queue
In reply to Bart Van AsscheAvoid that request_fn gets invoked after queue draining finished. blk_cleanup_queue() callers expect that request handling has finished once this function has returned so request_fn must not get invoked after blk_cleanup_queue() finished.
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Chanho Min <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- block/blk-core.c | 25 ++++++++++++++++++++++++- block/blk-exec.c | 2 +- block/blk.h | 2 ++ include/linux/blkdev.h | 2 ++ 4 files changed, 29 insertions(+), 2 deletions(-)
block/blk-core.c +24 -1
@@ -293,6 +293,25 @@ void blk_sync_queue(struct request_queue *q)EXPORT_SYMBOL (blk_sync_queue);/** +* __blk_run_queue_uncond - run a queue whether or not it has been stopped +* @q: The queue to run +* +* Description: +* Invoke request handling on a queue if there are any pending requests. +* May be used to restart request handling after a request has completed. +* This variant runs the queue whether or not the queue has been +* stopped. Must be called with the queue lock held and interrupts +* disabled. See also @blk_run_queue. +*/ +void __blk_run_queue_uncond (struct request_queue *q) +{ +if (unlikely (blk_queue_dead (q))) +return ; + + q->request_fn (q); +} + +/** * __blk_run_queue - run a single device queue * @q: The queue to run *
@@ -305,7 +324,7 @@ void __blk_run_queue(struct request_queue *q)if (unlikely (blk_queue_stopped (q)))return ; - q->request_fn (q); +__blk_run_queue_uncond (q); }EXPORT_SYMBOL (__blk_run_queue);
@@ -508,6 +527,10 @@ void blk_cleanup_queue(struct request_queue *q)/* drain all requests queued before DEAD marking */ blk_drain_queue (q, true); +spin_lock_irq (lock); +queue_flag_set (QUEUE_FLAG_DEAD , q); +spin_unlock_irq (lock); +/* @q won't process any more request, flush async actions */ del_timer_sync (&q->backing_dev_info.laptop_mode_wb_timer);blk_sync_queue (q);
block/blk-exec.c +1 -1
@@ -72,7 +72,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,__blk_run_queue (q);/* the queue is stopped so it won't be run */ if (rq->cmd_type ==REQ_TYPE_PM_RESUME ) - q->request_fn (q); +__blk_run_queue_uncond (q);spin_unlock_irq (q->queue_lock); }EXPORT_SYMBOL_GPL (blk_execute_rq_nowait);
block/blk.h +2 -0
@@ -145,6 +145,8 @@ int blk_try_merge(struct request *rq, struct bio *bio);void blk_queue_congestion_threshold (struct request_queue *q); +void __blk_run_queue_uncond (struct request_queue *q); +int blk_dev_init (void );
include/linux/blkdev.h +2 -0
@@ -451,6 +451,7 @@ struct request_queue {#define QUEUE_FLAG_ADD_RANDOM 16/* Contributes to random pool */ #define QUEUE_FLAG_SECDISCARD 17/* supports SECDISCARD */ #define QUEUE_FLAG_SAME_FORCE 18/* force complete on same CPU */ +#define QUEUE_FLAG_DEAD 19/* queue tear-down finished */ #define QUEUE_FLAG_DEFAULT ((1 << QUEUE_FLAG_IO_STAT) | \ (1 << QUEUE_FLAG_STACKABLE) | \
@@ -521,6 +522,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)#define blk_queue_tagged (q) test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)#define blk_queue_stopped (q) test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)#define blk_queue_dying (q) test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags) +#define blk_queue_dead (q) test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)#define blk_queue_bypass (q) test_bit(QUEUE_FLAG_BYPASS, &(q)->queue_flags)#define blk_queue_nomerges (q) test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)#define blk_queue_noxmerges (q) \
Signature · 1 line
1.7.10.4
PATCH v3 3/3 Make blk_cleanup_queue() wait until request_fn finished
In reply to Bart Van AsscheSome request_fn implementations, e.g. scsi_request_fn(), unlock the queue lock. Make sure that blk_cleanup_queue() waits until all active request_fn invocations have finished. This fixes a potential use-after-free at the end of scsi_request_fn().
Reported-by Chanho Min <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
--- block/blk-core.c | 7 +++++-- drivers/scsi/scsi_lib.c | 10 +--------- include/linux/blkdev.h | 5 +++++ 3 files changed, 11 insertions(+), 11 deletions(-)
block/blk-core.c +5 -2
@@ -308,7 +308,9 @@ void __blk_run_queue_uncond(struct request_queue *q)if (unlikely (blk_queue_dead (q)))return ; + q->request_fn_active++; q->request_fn (q); + q->request_fn_active--; }/**
@@ -407,6 +409,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)__blk_run_queue (q); drain |= q->nr_rqs_elvpriv; + drain |= q->request_fn_active;/* * Unfortunately, requests are queued at and tracked from
@@ -494,8 +497,8 @@ EXPORT_SYMBOL_GPL(blk_queue_bypass_end);* blk_cleanup_queue - shutdown a request queue * @q: request queue to shutdown * -* Mark @q DEAD, drain all pending requests, destroy and put it. All -* future requests will be failed immediately with -ENODEV. +* Mark @q as dying, drain all pending requests, mark @q as dead, destroy and +* put it. All future requests will be failed immediately with -ENODEV. */ void blk_cleanup_queue (struct request_queue *q) {
drivers/scsi/scsi_lib.c +1 -9
@@ -1517,10 +1517,6 @@ static void scsi_request_fn(struct request_queue *q)struct scsi_cmnd *cmd;struct request *req; -if (!get_device (&sdev->sdev_gendev)) -/* We must be tearing the block queue down already */ -return ; -/* * To start with, we keep looping until the queue is empty, or until * the host is no longer able to accept any more requests.
@@ -1629,11 +1625,7 @@ out_delay:if (sdev->device_busy ==0 )blk_delay_queue (q,SCSI_QUEUE_DELAY ); out: -/* must be careful here...if we trigger the ->remove() function -* we cannot be holding the q lock */ -spin_unlock_irq (q->queue_lock); -put_device (&sdev->sdev_gendev); -spin_lock_irq (q->queue_lock); + ; }u64 scsi_calculate_bounce_limit (struct Scsi_Host *shost)
include/linux/blkdev.h +5 -0
@@ -377,6 +377,11 @@ struct request_queue {unsigned int nr_sorted;unsigned int in_flight[2 ]; +/* +* Number of active request_fn() calls for those request_fn() +* implementations that unlock the queue_lock, e.g. scsi_request_fn(). +*/ +unsigned int request_fn_active;unsigned int rq_timeout;struct timer_list timeout;
Signature · 1 line
1.7.10.4
Re: [PATCH 3/3] Make blk_cleanup_queue() wait until request_fn finished
Quoted 66 lines · Bart Van Assche
Some
request_fnimplementations, e.g.scsi_request_fn(), unlock the queue lock. Make sure thatblk_cleanup_queue()waits until all activerequest_fninvocations have finished. This fixes a potential use-after-free at the end ofscsi_request_fn().Reported-by Chanho Min <[email protected]>Cc James Bottomley <[email protected]>Cc Mike Christie <[email protected]>Cc Jens Axboe <[email protected]>Cc Tejun Heo <[email protected]>Signed-off-by Bart Van Assche <[email protected]>--- block/blk-core.c | 7 +++++-- drivers/scsi/scsi_lib.c | 10 +--------- include/linux/blkdev.h | 5 +++++ 3 files changed, 11 insertions(+), 11 deletions(-)block/blk-core.c +5
-2@@ -308,7 +308,9 @@ void __blk_run_queue_uncond(struct request_queue *q)if (unlikely (blk_queue_dead (q)))return ; + q->request_fn_active++; q->request_fn (q); + q->request_fn_active--; }/** @@ -407,6 +409,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)__blk_run_queue (q); drain |= q->nr_rqs_elvpriv; + drain |= q->request_fn_active;/* * Unfortunately, requests are queued at and tracked from @@ -494,8 +497,8 @@ EXPORT_SYMBOL_GPL(blk_queue_bypass_end);* blk_cleanup_queue - shutdown a request queue * @q: request queue to shutdown * -* Mark @q DEAD, drain all pending requests, destroy and put it. All -* future requests will be failed immediately with -ENODEV. +* Mark @q as dying, drain all pending requests, mark @q as dead, destroy and +* put it. All future requests will be failed immediately with -ENODEV. */ void blk_cleanup_queue (struct request_queue *q) {drivers/scsi/scsi_lib.c +0
-4@@ -1517,10 +1517,6 @@ static void scsi_request_fn(struct request_queue *q)struct scsi_cmnd *cmd;struct request *req; -if (!get_device (&sdev->sdev_gendev)) -/* We must be tearing the block queue down already */ -return ; -/* * To start with, we keep looping until the queue is empty, or until * the host is no longer able to accept any more requests.
drivers/scsi/scsi_lib.c out_delay: · line 1628
Quoted 9 lines
drivers/scsi/scsi_lib.c +1
-5@@ -1629,11 +1625,7 @@ out_delay:if (sdev->device_busy ==0 )blk_delay_queue (q,SCSI_QUEUE_DELAY ); out: -/* must be careful here...if we trigger the ->remove() function -* we cannot be holding the q lock */ -spin_unlock_irq (q->queue_lock); -put_device (&sdev->sdev_gendev); -spin_lock_irq (q->queue_lock); + ;
Any reason to keep this "out:" label now that it has no effect?
--
Dan
Re: [PATCH 3/3] Make blk_cleanup_queue() wait until request_fn finished
Quoted 21 lines · Dan Williams
Quoted 20 lines · Bart Van Assche
[ ... ]
drivers/scsi/scsi_lib.c +0
-4@@ -1517,10 +1517,6 @@ static void scsi_request_fn(struct request_queue *q)struct scsi_cmnd *cmd;struct request *req; -if (!get_device (&sdev->sdev_gendev)) -/* We must be tearing the block queue down already */ -return ; -/* * To start with, we keep looping until the queue is empty, or until * the host is no longer able to accept any more requests. @@ -1629,11 +1625,7 @@ out_delay:if (sdev->device_busy ==0 )blk_delay_queue (q,SCSI_QUEUE_DELAY ); out:
Quoted 8 lines
drivers/scsi/scsi_lib.c out_delay: · line 1628
Quoted 6 lines
drivers/scsi/scsi_lib.c +1
-5-/* must be careful here...if we trigger the ->remove() function -* we cannot be holding the q lock */ -spin_unlock_irq (q->queue_lock); -put_device (&sdev->sdev_gendev); -spin_lock_irq (q->queue_lock); + ;Any reason to keep this "out:" label now that it has no effect?
Some people prefer a single-exit style for kernel code since that style makes it easy to add cleanup code for resources allocated inside the function itself. I don't have a strong opinion about this though.
Note: after I posted this patch series I noticed that patch 2/3 leaves a (small) race window. I'm currently testing this follow-up patch:
block/blk-core.c +3 -4
@@ -425,6 +425,9 @@ void blk_drain_queue(struct request_queue *q, bool drain_all) } } +if (!drain &&blk_queue_dying (q)) +queue_flag_set (QUEUE_FLAG_DEAD , q); +spin_unlock_irq (q->queue_lock);if (!drain)
@@ -525,10 +528,6 @@ void blk_cleanup_queue(struct request_queue *q)/* drain all requests queued before DEAD marking */ blk_drain_queue (q, true); -spin_lock_irq (lock); -queue_flag_set (QUEUE_FLAG_DEAD , q); -spin_unlock_irq (lock); -/* @q won't process any more request, flush async actions */ del_timer_sync (&q->backing_dev_info.laptop_mode_wb_timer);blk_sync_queue (q);