[PATCH V2] brd: check parameter validation before register_blkdev func

[PATCH V2] brd: check parameter validation before register_blkdev func

[Zhiqiang Liu]

In brd_init func, rd_nr num of brd_device are firstly allocated
and add in brd_devices, then brd_devices are traversed to add each
brd_device by calling add_disk func. When allocating brd_device,
the disk->first_minor is set to i * max_part, if rd_nr * max_part
is larger than MINORMASK, two different brd_device may have the same
devt, then only one of them can be successfully added.
when rmmod brd.ko, it will cause oops when calling brd_exit.

Follow those steps:
  # modprobe brd rd_nr=3 rd_size=102400 max_part=1048576
  # rmmod brd
then, the oops will appear.

Oops log:
[  726.613722] Call trace:
[  726.614175]  kernfs_find_ns+0x24/0x130
[  726.614852]  kernfs_find_and_get_ns+0x44/0x68
[  726.615749]  sysfs_remove_group+0x38/0xb0
[  726.616520]  blk_trace_remove_sysfs+0x1c/0x28
[  726.617320]  blk_unregister_queue+0x98/0x100
[  726.618105]  del_gendisk+0x144/0x2b8
[  726.618759]  brd_exit+0x68/0x560 [brd]
[  726.619501]  __arm64_sys_delete_module+0x19c/0x2a0
[  726.620384]  el0_svc_common+0x78/0x130
[  726.621057]  el0_svc_handler+0x38/0x78
[  726.621738]  el0_svc+0x8/0xc
[  726.622259] Code: aa0203f6 aa0103f7 aa1e03e0 d503201f (7940e260)

Here, we add brd_check_par_valid func to check parameter
validation before register_blkdev func.

--
V1->V2: add more checks in brd_check_par_valid as suggested by Ming Lei.

Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
---
 drivers/block/brd.c | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index df8103dd40ac..f211ee7d32b3 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -330,16 +330,16 @@ static const struct block_device_operations brd_fops = {
 /*
  * And now the modules code and kernel interface.
  */
-static int rd_nr = CONFIG_BLK_DEV_RAM_COUNT;
-module_param(rd_nr, int, 0444);
+static unsigned int rd_nr = CONFIG_BLK_DEV_RAM_COUNT;
+module_param(rd_nr, uint, 0444);
 MODULE_PARM_DESC(rd_nr, "Maximum number of brd devices");

 unsigned long rd_size = CONFIG_BLK_DEV_RAM_SIZE;
 module_param(rd_size, ulong, 0444);
 MODULE_PARM_DESC(rd_size, "Size of each RAM disk in kbytes.");

-static int max_part = 1;
-module_param(max_part, int, 0444);
+static unsigned int max_part = 1;
+module_param(max_part, uint, 0444);
 MODULE_PARM_DESC(max_part, "Num Minors to reserve between devices");

 MODULE_LICENSE("GPL");
@@ -468,10 +468,31 @@ static struct kobject *brd_probe(dev_t dev, int *part, void *data)
 	return kobj;
 }

+static inline int brd_check_par_valid(void)
+{
+	if (unlikely(!rd_nr))
+		rd_nr = 1;
+
+	if (unlikely(!max_part))
+		max_part = 1;
+
+	if (max_part > DISK_MAX_PARTS) {
+		pr_info("brd: max_part can't be larger than %d, reset max_part = %d.\n",
+			DISK_MAX_PARTS, DISK_MAX_PARTS);
+		max_part = DISK_MAX_PARTS;
+	}
+
+	if (rd_nr > MINORMASK / max_part)
+		return -EINVAL;
+
+	return 0;
+
+}
+
 static int __init brd_init(void)
 {
 	struct brd_device *brd, *next;
-	int i;
+	int i, ret;

 	/*
 	 * brd module now has a feature to instantiate underlying device
@@ -488,11 +509,15 @@ static int __init brd_init(void)
 	 *	dynamically.
 	 */

+	ret = brd_check_par_valid();
+	if (ret) {
+		pr_err("brd: invalid parameter setting!!!\n");
+		return ret;
+	}
+
 	if (register_blkdev(RAMDISK_MAJOR, "ramdisk"))
 		return -EIO;

-	if (unlikely(!max_part))
-		max_part = 1;

 	for (i = 0; i < rd_nr; i++) {
 		brd = brd_alloc(i);
-- 
2.19.1

Re: [PATCH V2] brd: check parameter validation before register_blkdev func

[Ming Lei]

On Mon, Jan 13, 2020 at 09:43:23PM +0800, Zhiqiang Liu wrote:
> In brd_init func, rd_nr num of brd_device are firstly allocated
> and add in brd_devices, then brd_devices are traversed to add each
> brd_device by calling add_disk func. When allocating brd_device,
> the disk->first_minor is set to i * max_part, if rd_nr * max_part
> is larger than MINORMASK, two different brd_device may have the same
> devt, then only one of them can be successfully added.

It is just because disk->first_minor is >= 0x100000, then same dev_t
can be allocated in blk_alloc_devt().

	MKDEV(disk->major, disk->first_minor + part->partno)

But block layer does support extended dynamic devt allocation, and brd
sets flag of GENHD_FL_EXT_DEVT too.

So I think the correct fix is to fallback to extended dynamic allocation
when running out of consecutive minor space.

How about the following approach?

And of course, ext devt allocation may fail too, but that is another
generic un-solved issue: error handling isn't done for adding disk.

diff --git a/drivers/block/brd.c b/drivers/block/brd.c
index a8730cc4db10..9aa7ce7c9abf 100644
--- a/drivers/block/brd.c
+++ b/drivers/block/brd.c
@@ -398,7 +398,16 @@ static struct brd_device *brd_alloc(int i)
 	if (!disk)
 		goto out_free_queue;
 	disk->major		= RAMDISK_MAJOR;
-	disk->first_minor	= i * max_part;
+
+	/*
+	 * Clear .minors when running out of consecutive minor space since
+	 * GENHD_FL_EXT_DEVT is set, and we can allocate from extended devt
+	 */
+	if ((i * disk->minors) & ~MINORMASK)
+		disk->minors = 0;
+	else
+		disk->first_minor	= i * disk->minors;
+
 	disk->fops		= &brd_fops;
 	disk->private_data	= brd;
 	disk->flags		= GENHD_FL_EXT_DEVT;



Thanks, 
Ming

Re: [PATCH V2] brd: check parameter validation before register_blkdev func

[Ming Lei]

On Tue, Jan 14, 2020 at 05:16:57PM +0800, Ming Lei wrote:
> On Mon, Jan 13, 2020 at 09:43:23PM +0800, Zhiqiang Liu wrote:
> > In brd_init func, rd_nr num of brd_device are firstly allocated
> > and add in brd_devices, then brd_devices are traversed to add each
> > brd_device by calling add_disk func. When allocating brd_device,
> > the disk->first_minor is set to i * max_part, if rd_nr * max_part
> > is larger than MINORMASK, two different brd_device may have the same
> > devt, then only one of them can be successfully added.
> 
> It is just because disk->first_minor is >= 0x100000, then same dev_t
> can be allocated in blk_alloc_devt().
> 
> 	MKDEV(disk->major, disk->first_minor + part->partno)
> 
> But block layer does support extended dynamic devt allocation, and brd
> sets flag of GENHD_FL_EXT_DEVT too.
> 
> So I think the correct fix is to fallback to extended dynamic allocation
> when running out of consecutive minor space.
> 
> How about the following approach?
> 
> And of course, ext devt allocation may fail too, but that is another
> generic un-solved issue: error handling isn't done for adding disk.
> 
> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
> index a8730cc4db10..9aa7ce7c9abf 100644
> --- a/drivers/block/brd.c
> +++ b/drivers/block/brd.c
> @@ -398,7 +398,16 @@ static struct brd_device *brd_alloc(int i)
>  	if (!disk)
>  		goto out_free_queue;
>  	disk->major		= RAMDISK_MAJOR;
> -	disk->first_minor	= i * max_part;
> +
> +	/*
> +	 * Clear .minors when running out of consecutive minor space since
> +	 * GENHD_FL_EXT_DEVT is set, and we can allocate from extended devt
> +	 */
> +	if ((i * disk->minors) & ~MINORMASK)
> +		disk->minors = 0;
> +	else
> +		disk->first_minor	= i * disk->minors;
> +
>  	disk->fops		= &brd_fops;
>  	disk->private_data	= brd;
>  	disk->flags		= GENHD_FL_EXT_DEVT;

But still suggest to limit 'max_part' <= 256, and the name is actually
misleading, which just reserves consecutive minors.

However, I don't think it is a good idea to add limit on device number.


Thanks,
Ming

Re: [PATCH V2] brd: check parameter validation before register_blkdev func

[Zhiqiang Liu]

On 2020/1/14 17:45, Ming Lei wrote:
> On Tue, Jan 14, 2020 at 05:16:57PM +0800, Ming Lei wrote:
>> On Mon, Jan 13, 2020 at 09:43:23PM +0800, Zhiqiang Liu wrote:
>>> In brd_init func, rd_nr num of brd_device are firstly allocated
>>> and add in brd_devices, then brd_devices are traversed to add each
>>> brd_device by calling add_disk func. When allocating brd_device,
>>> the disk->first_minor is set to i * max_part, if rd_nr * max_part
>>> is larger than MINORMASK, two different brd_device may have the same
>>> devt, then only one of them can be successfully added.
>>
>> It is just because disk->first_minor is >= 0x100000, then same dev_t
>> can be allocated in blk_alloc_devt().
>>
>> 	MKDEV(disk->major, disk->first_minor + part->partno)
>>
>> But block layer does support extended dynamic devt allocation, and brd
>> sets flag of GENHD_FL_EXT_DEVT too.
>>
>> So I think the correct fix is to fallback to extended dynamic allocation
>> when running out of consecutive minor space.
>>
>> How about the following approach?
>>
>> And of course, ext devt allocation may fail too, but that is another
>> generic un-solved issue: error handling isn't done for adding disk.
>>
>> diff --git a/drivers/block/brd.c b/drivers/block/brd.c
>> index a8730cc4db10..9aa7ce7c9abf 100644
>> --- a/drivers/block/brd.c
>> +++ b/drivers/block/brd.c
>> @@ -398,7 +398,16 @@ static struct brd_device *brd_alloc(int i)
>>  	if (!disk)
>>  		goto out_free_queue;
>>  	disk->major		= RAMDISK_MAJOR;
>> -	disk->first_minor	= i * max_part;
>> +
>> +	/*
>> +	 * Clear .minors when running out of consecutive minor space since
>> +	 * GENHD_FL_EXT_DEVT is set, and we can allocate from extended devt
>> +	 */
>> +	if ((i * disk->minors) & ~MINORMASK)
>> +		disk->minors = 0;
>> +	else
>> +		disk->first_minor	= i * disk->minors;
>> +
>>  	disk->fops		= &brd_fops;
>>  	disk->private_data	= brd;
>>  	disk->flags		= GENHD_FL_EXT_DEVT;
> 
> But still suggest to limit 'max_part' <= 256, and the name is actually
> misleading, which just reserves consecutive minors.
> 
> However, I don't think it is a good idea to add limit on device number.
> 

Thanks for your patient replyI will resend the v3 patch as your suggestion.
Changes in v3:
	1)clear .minors when running out of consecutive minor space in brd_alloc.
	2)remove limit of rd_nr