On Fri, Sep 13, 2019 at 11:57:17AM -0300, André Almeida wrote:
> 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.
Why is that an improvement? I agree it's an uninteresting thing to ask
for, but I don't see a compelling need to fail the module load because
of it.
> 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.
If you ask it to create 4 billion devices, what happens? Obviously we'll
run out of dev_t at some point ...
