[PATCH 1/2] ataflop: fix error handling in atari_floppy_init()

[PATCH 1/2] ataflop: fix error handling in atari_floppy_init()

[Dan Carpenter]

Smatch complains that there is an off by one if the allocation fails in:

	DMABuffer = atari_stram_alloc(BUFFER_SIZE+512, "ataflop");

In that situation, "i" would be point to one element beyond the end of
the unit[] array.

There is a second bug because the error handling calls
blk_mq_free_tag_set(&unit[i].tag_set); regardless of whether
"disk->queue" is NULL or non-NULL.  So if blk_mq_init_sq_queue() fails,
then that means unit[i].tag_set->tags is NULL and it leads to an Oops.

It's easiest to call put_disk() before the goto to clean up the partial
iteration.  Then the earlier unit[] elements are fully allocated so we
can remove the checks whether "disk->queue" is NULL and the code is
simpler.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---

I hope the Atari floppy disk users are appropriately grateful for all
the love and effort we put into their software...

 drivers/block/ataflop.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c
index f88b4c26d422..313064b1005c 100644
--- a/drivers/block/ataflop.c
+++ b/drivers/block/ataflop.c
@@ -1982,6 +1982,7 @@ static int __init atari_floppy_init (void)
 							   &ataflop_mq_ops, 2,
 							   BLK_MQ_F_SHOULD_MERGE);
 		if (IS_ERR(unit[i].disk->queue)) {
+			put_disk(unit[i].disk);
 			ret = PTR_ERR(unit[i].disk->queue);
 			unit[i].disk->queue = NULL;
 			goto err;
@@ -2033,18 +2034,13 @@ static int __init atari_floppy_init (void)
 	return 0;
 
 err:
-	do {
+	while (--i >= 0) {
 		struct gendisk *disk = unit[i].disk;
 
-		if (disk) {
-			if (disk->queue) {
-				blk_cleanup_queue(disk->queue);
-				disk->queue = NULL;
-			}
-			blk_mq_free_tag_set(&unit[i].tag_set);
-			put_disk(unit[i].disk);
-		}
-	} while (i--);
+		blk_cleanup_queue(disk->queue);
+		blk_mq_free_tag_set(&unit[i].tag_set);
+		put_disk(unit[i].disk);
+	}
 
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 	return ret;
-- 
2.11.0

[PATCH 2/2] blk-mq: Add a NULL check in blk_mq_free_map_and_requests()

[Dan Carpenter]

I recently found some code which called blk_mq_free_map_and_requests()
with a NULL set->tags pointer.  I fixed the caller, but it seems like a
good idea to add a NULL check here as well.  Now we can call:

	blk_mq_free_tag_set(set);
	blk_mq_free_tag_set(set);

twice in a row and it's harmless.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index a82830f39933..5f4b93f424b4 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2341,7 +2341,7 @@ static bool __blk_mq_alloc_rq_map(struct blk_mq_tag_set *set, int hctx_idx)
 static void blk_mq_free_map_and_requests(struct blk_mq_tag_set *set,
 					 unsigned int hctx_idx)
 {
-	if (set->tags[hctx_idx]) {
+	if (set->tags && set->tags[hctx_idx]) {
 		blk_mq_free_rqs(set, set->tags[hctx_idx], hctx_idx);
 		blk_mq_free_rq_map(set->tags[hctx_idx]);
 		set->tags[hctx_idx] = NULL;
-- 
2.11.0

Re: [PATCH 1/2] ataflop: fix error handling in atari_floppy_init()

[Jens Axboe]

On 11/29/18 3:55 AM, Dan Carpenter wrote:
> Smatch complains that there is an off by one if the allocation fails in:
> 
> 	DMABuffer = atari_stram_alloc(BUFFER_SIZE+512, "ataflop");
> 
> In that situation, "i" would be point to one element beyond the end of
> the unit[] array.
> 
> There is a second bug because the error handling calls
> blk_mq_free_tag_set(&unit[i].tag_set); regardless of whether
> "disk->queue" is NULL or non-NULL.  So if blk_mq_init_sq_queue() fails,
> then that means unit[i].tag_set->tags is NULL and it leads to an Oops.
> 
> It's easiest to call put_disk() before the goto to clean up the partial
> iteration.  Then the earlier unit[] elements are fully allocated so we
> can remove the checks whether "disk->queue" is NULL and the code is
> simpler.

Applied, thanks.

> I hope the Atari floppy disk users are appropriately grateful for all
> the love and effort we put into their software...

I'm sure that one person has you on the xmas shipping list.

-- 
Jens Axboe

Re: [PATCH 2/2] blk-mq: Add a NULL check in blk_mq_free_map_and_requests()

[Jens Axboe]

On 11/29/18 3:56 AM, Dan Carpenter wrote:
> I recently found some code which called blk_mq_free_map_and_requests()
> with a NULL set->tags pointer.  I fixed the caller, but it seems like a
> good idea to add a NULL check here as well.  Now we can call:
> 
> 	blk_mq_free_tag_set(set);
> 	blk_mq_free_tag_set(set);
> 
> twice in a row and it's harmless.

Applied, thanks.

-- 
Jens Axboe