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

[Randy Dunlap]

On 3/16/19 12:31 PM, Jens Axboe wrote:
> 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.
> 

OK, this one works for both pf and pcd.
By "works" I mean that the driver init function runs and exits without
causing a BUG or GP fault etc.
Not that I have any such hardware.

Tested-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>

Thanks.

> 
> 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);
>  	}
>  }
>  
> 


-- 
~Randy