block: fix possible race on blk_get_queue()
diff mbox series

Message ID 20200729015101.31534-1-mcgrof@kernel.org
State New
Headers show
Series
  • block: fix possible race on blk_get_queue()
Related show

Commit Message

Luis Chamberlain July 29, 2020, 1:51 a.m. UTC
The queue can flip to dying after we check if it is dying,
and then we call __blk_get_queue(). This is a purely
theoretical race, but just fix it. We do this by
Using the atomic kobject_get_unless_zero() first, and
*then* check if the queue is dying *after*.

This issue was found while doing patch review on the
recent blktrace fixes [0].

[0] https://lore.kernel.org/linux-block/20200415123434.GU11244@42.do-not-panic.com/

Reported-by: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---

This goes tested against blktest without finding a regression.

 block/blk-core.c | 14 ++++++++++----
 block/blk.h      |  5 +++--
 2 files changed, 13 insertions(+), 6 deletions(-)

Comments

Bart Van Assche July 29, 2020, 2:41 a.m. UTC | #1
On 2020-07-28 18:51, Luis Chamberlain wrote:
> diff --git a/block/blk-core.c b/block/blk-core.c
> index d9d632639bd1..febdd8e8d409 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -605,12 +605,18 @@ EXPORT_SYMBOL(blk_alloc_queue);
>   */
>  bool blk_get_queue(struct request_queue *q)
>  {
> -	if (likely(!blk_queue_dying(q))) {
> -		__blk_get_queue(q);
> -		return true;
> +	struct kobject *obj;
> +
> +	obj = __blk_get_queue(q);
> +	if (!obj)
> +		return false;
> +
> +	if (unlikely(blk_queue_dying(q))) {
> +		blk_put_queue(q);
> +		return false;
>  	}
>  
> -	return false;
> +	return true;
>  }

This change is not sufficient to prevent that the QUEUE_FLAG_DYING flag
is set immediately after this function returns. I propose not to modify
this function but instead to add a comment that is the responsibility of
the caller to prevent that such a race condition occurs.

> -static inline void __blk_get_queue(struct request_queue *q)
> +static inline struct kobject * __must_check
> +__blk_get_queue(struct request_queue *q)
>  {
> -	kobject_get(&q->kobj);
> +	return kobject_get_unless_zero(&q->kobj);
>  }

If a function passes a queue pointer to another function that calls
blk_get_queue() then the caller should guarantee that 'q' is valid
during the entire duration of the call. In other words, I'm not sure the
above change is an improvement.

Thanks,

Bart.

Patch
diff mbox series

diff --git a/block/blk-core.c b/block/blk-core.c
index d9d632639bd1..febdd8e8d409 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -605,12 +605,18 @@  EXPORT_SYMBOL(blk_alloc_queue);
  */
 bool blk_get_queue(struct request_queue *q)
 {
-	if (likely(!blk_queue_dying(q))) {
-		__blk_get_queue(q);
-		return true;
+	struct kobject *obj;
+
+	obj = __blk_get_queue(q);
+	if (!obj)
+		return false;
+
+	if (unlikely(blk_queue_dying(q))) {
+		blk_put_queue(q);
+		return false;
 	}
 
-	return false;
+	return true;
 }
 EXPORT_SYMBOL(blk_get_queue);
 
diff --git a/block/blk.h b/block/blk.h
index 49e2928a1632..bdbc9b084d5b 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -39,9 +39,10 @@  blk_get_flush_queue(struct request_queue *q, struct blk_mq_ctx *ctx)
 	return blk_mq_map_queue(q, REQ_OP_FLUSH, ctx)->fq;
 }
 
-static inline void __blk_get_queue(struct request_queue *q)
+static inline struct kobject * __must_check
+__blk_get_queue(struct request_queue *q)
 {
-	kobject_get(&q->kobj);
+	return kobject_get_unless_zero(&q->kobj);
 }
 
 static inline bool