Re: [PATCH 3/3] null_blk: validated the number of devices

From: "André Almeida" <andrealmeid@collabora.com>
To: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>,
	Matthew Wilcox <willy@infradead.org>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"corbet@lwn.net" <corbet@lwn.net>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"m@bjorling.me" <m@bjorling.me>,
	"kernel@collabora.com" <kernel@collabora.com>,
	"krisman@collabora.com" <krisman@collabora.com>
Subject: Re: [PATCH 3/3] null_blk: validated the number of devices
Date: Fri, 13 Sep 2019 11:57:17 -0300	[thread overview]
Message-ID: <d55554f5-7212-c5fc-dbb0-4269be913aaa@collabora.com> (raw)
In-Reply-To: <DM6PR04MB5754177B405819C802549AE686B00@DM6PR04MB5754.namprd04.prod.outlook.com>

On 9/12/19 7:20 PM, Chaitanya Kulkarni wrote:
> On 09/12/2019 03:09 PM, André Almeida wrote:
>> Hello Matthew,
>>
>> On 9/12/19 1:19 PM, Matthew Wilcox wrote:
>>> On Wed, Sep 11, 2019 at 11:46:36AM -0300, André Almeida wrote:
>>>>
>>>> -static int nr_devices = 1;
>>>> +static unsigned int nr_devices = 1;
>>>>   module_param(nr_devices, int, 0444);
>>>
>>> ^^^ you forgot to change the module_param to match
>>>
>>>> +	if (!nr_devices) {
>>>> +		pr_err("null_blk: invalid number of devices\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> I don't think this is necessary.
>>>
>>
>> Could you explain why you don't think is necessary? As I see, the module
>> can't be used without any /dev/nullb* device, so why we should load it?
>>
>> Thanks,
>> 	André
>>
> 
> I think Matthew is right here. I think module can be loaded with 
> nr_devices=0.
> 
> Did you get a chance to test nr_device=0 condition ?
> 

Yes. It says "module loaded" and lsmod shows that it is loaded indeed.
But you don't have any /dev/nullb*, so you can't do much with the module.

With this patch, it refuses to load the module.

> Also, did you get a chance to test this patch with all the
> possible conditions ?
>

I did not tested with all possible conditions, but I tested with the
following ones:

* Using a negative number of devices:
	- Previously, it would alloc and add a huge number of devices until the
system gets out of memory
	- With module_param as uint, it will throw a "invalid for parameter
`nr_devices'" and refuse to load

* Using a range of values (1, 10, 100, 1000):
	- It will works as expect, creating some /dev/nullbX accordingly with
your parameter. Works fine with and without this patch.