| Message ID | 20190911144636.226945-3-andrealmeid@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] docs: block: null_blk: enhance document style | expand |
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.
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é
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 ? Also, did you get a chance to test this patch with all the possible conditions ?
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.
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 ...
On 9/13/19 12:12 PM, Matthew Wilcox wrote: > 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. > Indeed, failing is used when there is something wrong with the module, and this is not the case when (nr_devices == 0). I will remove this, thanks! >> 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 ... >
On 9/13/19 9:12 AM, Matthew Wilcox wrote: > 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. It also breaks the case of loading it, then setting up a new device through configfs.
On 09/13/2019 09:23 AM, Jens Axboe wrote: > It also breaks the case of loading it, then setting up a new device > through configfs. Yep, this is exactly I was asking nr_device=0 as modparam is allowed and membacked null_blk creation will fail with this check.
On 9/13/19 10:27 AM, Chaitanya Kulkarni wrote: > On 09/13/2019 09:23 AM, Jens Axboe wrote: >> It also breaks the case of loading it, then setting up a new device >> through configfs. > > Yep, this is exactly I was asking nr_device=0 as modparam is allowed > and membacked null_blk creation will fail with this check. > Yeah, it's totally broken...
diff --git a/drivers/block/null_blk_main.c b/drivers/block/null_blk_main.c index 7bc553ff7407..ab4b87677139 100644 --- a/drivers/block/null_blk_main.c +++ b/drivers/block/null_blk_main.c @@ -141,7 +141,7 @@ static int g_bs = 512; module_param_named(bs, g_bs, int, 0444); MODULE_PARM_DESC(bs, "Block size (in bytes)"); -static int nr_devices = 1; +static unsigned int nr_devices = 1; module_param(nr_devices, int, 0444); MODULE_PARM_DESC(nr_devices, "Number of devices to register"); @@ -1758,6 +1758,10 @@ static int __init null_init(void) pr_err("null_blk: legacy IO path no longer available\n"); return -EINVAL; } + if (!nr_devices) { + pr_err("null_blk: invalid number of devices\n"); + return -EINVAL; + } if (g_queue_mode == NULL_Q_MQ && g_use_per_node_hctx) { if (g_submit_queues != nr_online_nodes) { pr_warn("null_blk: submit_queues param is set to %u.\n",
There is no sense defining a negative number of devices, so change the type to unsigned. If the number of devices is 0, it is impossible to the userspace interact with the module, so there is no reasoning in loading it. Signed-off-by: André Almeida <andrealmeid@collabora.com> --- drivers/block/null_blk_main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)