Re: paride/pf.c: blk-mq use-after-free (kernel v5.0)

[Jens Axboe <axboe@kernel.dk>]

On 3/15/19 6:32 PM, Randy Dunlap wrote:
> On 3/15/19 9:33 AM, Jens Axboe wrote:
>> On 3/14/19 5:49 PM, Randy Dunlap wrote:
>>> On 3/14/19 4:43 PM, Jens Axboe wrote:
>>>> On 3/13/19 5:09 PM, Randy Dunlap wrote:
>>>>> On 3/11/19 6:34 PM, Randy Dunlap wrote:
>>>>>> On 3/11/19 6:25 PM, Randy Dunlap wrote:
>>>>>>> [Has this already been addressed/fixed?]>>
>>>>>>
>>>>>> Same bug occurs with paride/pcd.c driver.
>>>>>
>>>>> This still happens (in blk-mq) in v5.0-11053-gebc551f2b8f9 of Mar. 12, 2019,
>>>>> around 4pm PT.  [caused by paride: pf.c and pcd.c)
>>>>
>>>> I'll take a look at this, been busy with other stuff. How are you
>>>> reproducing this? I'm assuming you don't actually have any hardware :-)
>>>
>>> Right.  I just load the module (pf or pcd), unload it, and
>>> then load it again.
>>
>> Does this work?
>>
> 
> No.  Just loading the pf module gives this:

Missing clear of the queue. This one should be more complete.

To be fair, this was utterly broken since forever. It's just now apparent
since we complain about it. But pf/pcd was one big leak fest.


diff --git a/drivers/block/paride/pcd.c b/drivers/block/paride/pcd.c
index 96670eefaeb2..377a694dc228 100644
--- a/drivers/block/paride/pcd.c
+++ b/drivers/block/paride/pcd.c
@@ -749,8 +749,12 @@ static int pcd_detect(void)
 		return 0;
 
 	printk("%s: No CD-ROM drive found\n", name);
-	for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++)
+	for (unit = 0, cd = pcd; unit < PCD_UNITS; unit++, cd++) {
+		blk_cleanup_queue(cd->disk->queue);
+		cd->disk->queue = NULL;
+		blk_mq_free_tag_set(&cd->tag_set);
 		put_disk(cd->disk);
+	}
 	pi_unregister_driver(par_drv);
 	return -1;
 }
diff --git a/drivers/block/paride/pf.c b/drivers/block/paride/pf.c
index e92e7a8eeeb2..103b617cdc31 100644
--- a/drivers/block/paride/pf.c
+++ b/drivers/block/paride/pf.c
@@ -761,8 +761,12 @@ static int pf_detect(void)
 		return 0;
 
 	printk("%s: No ATAPI disk detected\n", name);
-	for (pf = units, unit = 0; unit < PF_UNITS; pf++, unit++)
+	for (pf = units, unit = 0; unit < PF_UNITS; pf++, unit++) {
+		blk_cleanup_queue(pf->disk->queue);
+		pf->disk->queue = NULL;
+		blk_mq_free_tag_set(&pf->tag_set);
 		put_disk(pf->disk);
+	}
 	pi_unregister_driver(par_drv);
 	return -1;
 }
@@ -1047,13 +1051,15 @@ static void __exit pf_exit(void)
 	int unit;
 	unregister_blkdev(major, name);
 	for (pf = units, unit = 0; unit < PF_UNITS; pf++, unit++) {
-		if (!pf->present)
-			continue;
-		del_gendisk(pf->disk);
+		if (pf->present)
+			del_gendisk(pf->disk);
+
 		blk_cleanup_queue(pf->disk->queue);
 		blk_mq_free_tag_set(&pf->tag_set);
 		put_disk(pf->disk);
-		pi_release(pf->pi);
+
+		if (pf->present)
+			pi_release(pf->pi);
 	}
 }
 

-- 
Jens Axboe