| Message ID | 20230410051352.36856-2-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2,1/1] null_blk: add moddule parameter check | expand |
On 4/10/23 14:13, Chaitanya Kulkarni wrote: > static bool g_virt_boundary = false; > module_param_named(virt_boundary, g_virt_boundary, bool, 0444); > MODULE_PARM_DESC(virt_boundary, "Require a virtual boundary for the device. Default: False"); > @@ -86,11 +113,13 @@ module_param_named(no_sched, g_no_sched, int, 0444); > MODULE_PARM_DESC(no_sched, "No io scheduler"); > > static int g_submit_queues = 1; > -module_param_named(submit_queues, g_submit_queues, int, 0444); > +NULL_PARAM(submit_queues, 1, INT_MAX); INT_MAX ? That is a little large... Why not using CONFIG_NR_CPUS ? Or CONFIG_NR_CPUS * 2 to leave some room for experiments. > +device_param_cb(submit_queues, &null_submit_queues_param_ops, &g_submit_queues, 0444); > MODULE_PARM_DESC(submit_queues, "Number of submission queues"); [...] > static int g_bs = 512; > -module_param_named(bs, g_bs, int, 0444); > +NULL_PARAM(bs, 1, PAGE_SIZE); Allowing min=1 here does not make sense. This should be 512. And you could add a "bool is_power_of_2" argument to null_param_store_int() to check that the value is a power of 2. > +device_param_cb(bs, &null_bs_param_ops, &g_bs, 0444); > MODULE_PARM_DESC(bs, "Block size (in bytes)"); [...] > static int g_hw_queue_depth = 64; > -module_param_named(hw_queue_depth, g_hw_queue_depth, int, 0444); > +NULL_PARAM(hw_qdepth, 1, INT_MAX); INT_MAX here is a little silly. Way too large to make any sense in my opinion. NVMe allows up to 65536, why not use the same value here ? > +device_param_cb(hw_queue_depth, &null_hw_qdepth_param_ops, &g_hw_queue_depth, 0444); > MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 64"); > > static bool g_use_per_node_hctx;
On 23/04/09 10:13PM, Chaitanya Kulkarni wrote: >Move null_param_store_int() function to the top of the code so that >it can be accessed by the new NULL_PARAM() macro. The macro takes >name of the module parameter, minimum value, and maximum value as >input, then creates specific callback functions and kernel_param >ops for each module parameter. This macro ultimately calls the >null_param_store_int() function, which returns an error if the >user-provided value is out of bounds(min,max). > >To prevent negative values from being set by the user for following >list of module parameters, uses NULL_PARM() macro to add user input >validation: > submit_queues > poll_queues > queue_mode > gb > max_sectors > irqmode > hw_qdepth > bs > >This commit improves the organization and safety of the code, making >it easier to maintain and preventing users from accidentally setting >negative values for the specified parameters. > >Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >--- > drivers/block/null_blk/main.c | 85 +++++++++++++++++------------------ > 1 file changed, 41 insertions(+), 44 deletions(-) > >diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >index bc2c58724df3..fcb2ce4f9dc8 100644 >--- a/drivers/block/null_blk/main.c >+++ b/drivers/block/null_blk/main.c >@@ -77,6 +77,33 @@ enum { > NULL_IRQ_TIMER = 2, > }; > >+static int null_param_store_int(const char *str, int *val, int min, int max) >+{ >+ int ret, new_val; >+ >+ ret = kstrtoint(str, 10, &new_val); >+ if (ret) >+ return -EINVAL; >+ >+ if (new_val < min || new_val > max) >+ return -EINVAL; >+ >+ *val = new_val; >+ return 0; >+} >+ >+#define NULL_PARAM(_name, _min, _max) \ >+static int null_param_##_name##_set(const char *s, \ >+ const struct kernel_param *kp) \ >+{ \ >+ return null_param_store_int(s, kp->arg, _min, _max); \ >+} \ >+ \ >+static const struct kernel_param_ops null_##_name##_param_ops = { \ >+ .set = null_param_##_name##_set, \ >+ .get = param_get_int, \ >+} >+ > static bool g_virt_boundary = false; > module_param_named(virt_boundary, g_virt_boundary, bool, 0444); > MODULE_PARM_DESC(virt_boundary, "Require a virtual boundary for the device. Default: False"); >@@ -86,11 +113,13 @@ module_param_named(no_sched, g_no_sched, int, 0444); > MODULE_PARM_DESC(no_sched, "No io scheduler"); > > static int g_submit_queues = 1; >-module_param_named(submit_queues, g_submit_queues, int, 0444); >+NULL_PARAM(submit_queues, 1, INT_MAX); >+device_param_cb(submit_queues, &null_submit_queues_param_ops, &g_submit_queues, 0444); > MODULE_PARM_DESC(submit_queues, "Number of submission queues"); > > static int g_poll_queues = 1; >-module_param_named(poll_queues, g_poll_queues, int, 0444); >+NULL_PARAM(poll_queues, 1, num_online_cpus()); >+device_param_cb(poll_queues, &null_poll_queues_param_ops, &g_poll_queues, 0444); > MODULE_PARM_DESC(poll_queues, "Number of IOPOLL submission queues"); > > static int g_home_node = NUMA_NO_NODE; >@@ -116,45 +145,23 @@ MODULE_PARM_DESC(init_hctx, "Fault injection to fail hctx init. init_hctx=<inter > #endif > > static int g_queue_mode = NULL_Q_MQ; >- >-static int null_param_store_val(const char *str, int *val, int min, int max) >-{ >- int ret, new_val; >- >- ret = kstrtoint(str, 10, &new_val); >- if (ret) >- return -EINVAL; >- >- if (new_val < min || new_val > max) >- return -EINVAL; >- >- *val = new_val; >- return 0; >-} >- >-static int null_set_queue_mode(const char *str, const struct kernel_param *kp) >-{ >- return null_param_store_val(str, &g_queue_mode, NULL_Q_BIO, NULL_Q_MQ); >-} >- >-static const struct kernel_param_ops null_queue_mode_param_ops = { >- .set = null_set_queue_mode, >- .get = param_get_int, >-}; >- >+NULL_PARAM(queue_mode, NULL_Q_BIO, NULL_Q_MQ); > device_param_cb(queue_mode, &null_queue_mode_param_ops, &g_queue_mode, 0444); > MODULE_PARM_DESC(queue_mode, "Block interface to use (0=bio,1=rq,2=multiqueue)"); > > static int g_gb = 250; >-module_param_named(gb, g_gb, int, 0444); >+NULL_PARAM(gb, 1, INT_MAX); This value gets converted to mb, for dev->size calculation in null_alloc_dev. I think either there should be a type conversion or this module parameter max value can be reduced to smaller value. >+device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); > MODULE_PARM_DESC(gb, "Size in GB"); > > static int g_bs = 512; >-module_param_named(bs, g_bs, int, 0444); >+NULL_PARAM(bs, 1, PAGE_SIZE); >+device_param_cb(bs, &null_bs_param_ops, &g_bs, 0444); > MODULE_PARM_DESC(bs, "Block size (in bytes)"); > > static int g_max_sectors; >-module_param_named(max_sectors, g_max_sectors, int, 0444); >+NULL_PARAM(max_sectors, 1, INT_MAX); >+device_param_cb(max_sectors, &null_max_sectors_param_ops, &g_max_sectors, 0444); > MODULE_PARM_DESC(max_sectors, "Maximum size of a command (in 512B sectors)"); > > static unsigned int nr_devices = 1; >@@ -174,18 +181,7 @@ module_param_named(shared_tag_bitmap, g_shared_tag_bitmap, bool, 0444); > MODULE_PARM_DESC(shared_tag_bitmap, "Use shared tag bitmap for all submission queues for blk-mq"); > > static int g_irqmode = NULL_IRQ_SOFTIRQ; >- >-static int null_set_irqmode(const char *str, const struct kernel_param *kp) >-{ >- return null_param_store_val(str, &g_irqmode, NULL_IRQ_NONE, >- NULL_IRQ_TIMER); >-} >- >-static const struct kernel_param_ops null_irqmode_param_ops = { >- .set = null_set_irqmode, >- .get = param_get_int, >-}; >- >+NULL_PARAM(irqmode, NULL_IRQ_NONE, NULL_IRQ_TIMER); > device_param_cb(irqmode, &null_irqmode_param_ops, &g_irqmode, 0444); > MODULE_PARM_DESC(irqmode, "IRQ completion handler. 0-none, 1-softirq, 2-timer"); > >@@ -194,7 +190,8 @@ module_param_named(completion_nsec, g_completion_nsec, ulong, 0444); > MODULE_PARM_DESC(completion_nsec, "Time in ns to complete a request in hardware. Default: 10,000ns"); > > static int g_hw_queue_depth = 64; >-module_param_named(hw_queue_depth, g_hw_queue_depth, int, 0444); >+NULL_PARAM(hw_qdepth, 1, INT_MAX); >+device_param_cb(hw_queue_depth, &null_hw_qdepth_param_ops, &g_hw_queue_depth, 0444); > MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 64"); > > static bool g_use_per_node_hctx; >-- Regards, Nitesh Shetty >2.29.0 >
On 4/11/23 02:47, Nitesh Shetty wrote: >> static int g_gb = 250; >> -module_param_named(gb, g_gb, int, 0444); >> +NULL_PARAM(gb, 1, INT_MAX); > > This value gets converted to mb, for dev->size calculation in > null_alloc_dev. I think either there should be a type conversion or > this module parameter max value can be reduced to smaller value. Yeah, good catch. it is multiplied by 1024, and assigned to dev->size which is an unsigned long. So that could overflow on 32-bits arch. So this needs some fixing. I would still allow a very large value as possible though, to allow testing for overflows. > >> +device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); >> MODULE_PARM_DESC(gb, "Size in GB"); Chaitanya, Another thing: did you check if setting all these arguments through configfs also gets the same min/max value treatment ? Ideally, we want both configuration interfaces (module args and configfs) to be equivalent. (Note: please use dlemoal@kernel.org. wdc.com addresses do not work right now)
On 4/10/23 22:29, Damien Le Moal wrote: > On 4/11/23 02:47, Nitesh Shetty wrote: >>> static int g_gb = 250; >>> -module_param_named(gb, g_gb, int, 0444); >>> +NULL_PARAM(gb, 1, INT_MAX); >> This value gets converted to mb, for dev->size calculation in >> null_alloc_dev. I think either there should be a type conversion or >> this module parameter max value can be reduced to smaller value. > Yeah, good catch. it is multiplied by 1024, and assigned to dev->size which is > an unsigned long. So that could overflow on 32-bits arch. So this needs some fixing. > > I would still allow a very large value as possible though, to allow testing for > overflows. will change the type in next version, but still keep the large value possible for that type. >>> +device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); >>> MODULE_PARM_DESC(gb, "Size in GB"); > Chaitanya, > > Another thing: did you check if setting all these arguments through configfs > also gets the same min/max value treatment ? Ideally, we want both configuration > interfaces (module args and configfs) to be equivalent. I'm trying to avoid that in the same patch, are you okay to add that in the same patch or a separate one ? > (Note: please use dlemoal@kernel.org. wdc.com addresses do not work right now) noticed that from bounced mails from hgst server, will fix it next git-send... -ck
On 4/11/23 15:40, Chaitanya Kulkarni wrote: > On 4/10/23 22:29, Damien Le Moal wrote: >> On 4/11/23 02:47, Nitesh Shetty wrote: >>>> static int g_gb = 250; >>>> -module_param_named(gb, g_gb, int, 0444); >>>> +NULL_PARAM(gb, 1, INT_MAX); >>> This value gets converted to mb, for dev->size calculation in >>> null_alloc_dev. I think either there should be a type conversion or >>> this module parameter max value can be reduced to smaller value. >> Yeah, good catch. it is multiplied by 1024, and assigned to dev->size which is >> an unsigned long. So that could overflow on 32-bits arch. So this needs some fixing. >> >> I would still allow a very large value as possible though, to allow testing for >> overflows. > > will change the type in next version, but still keep the large value > possible for that type. > >>>> +device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); >>>> MODULE_PARM_DESC(gb, "Size in GB"); >> Chaitanya, >> >> Another thing: did you check if setting all these arguments through configfs >> also gets the same min/max value treatment ? Ideally, we want both configuration >> interfaces (module args and configfs) to be equivalent. > > I'm trying to avoid that in the same patch, > are you okay to add that in the same patch or a separate one ? Separate patch is fine. > >> (Note: please use dlemoal@kernel.org. wdc.com addresses do not work right now) > > noticed that from bounced mails from hgst server, will fix it next > git-send... > > -ck > >
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index bc2c58724df3..fcb2ce4f9dc8 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -77,6 +77,33 @@ enum { NULL_IRQ_TIMER = 2, }; +static int null_param_store_int(const char *str, int *val, int min, int max) +{ + int ret, new_val; + + ret = kstrtoint(str, 10, &new_val); + if (ret) + return -EINVAL; + + if (new_val < min || new_val > max) + return -EINVAL; + + *val = new_val; + return 0; +} + +#define NULL_PARAM(_name, _min, _max) \ +static int null_param_##_name##_set(const char *s, \ + const struct kernel_param *kp) \ +{ \ + return null_param_store_int(s, kp->arg, _min, _max); \ +} \ + \ +static const struct kernel_param_ops null_##_name##_param_ops = { \ + .set = null_param_##_name##_set, \ + .get = param_get_int, \ +} + static bool g_virt_boundary = false; module_param_named(virt_boundary, g_virt_boundary, bool, 0444); MODULE_PARM_DESC(virt_boundary, "Require a virtual boundary for the device. Default: False"); @@ -86,11 +113,13 @@ module_param_named(no_sched, g_no_sched, int, 0444); MODULE_PARM_DESC(no_sched, "No io scheduler"); static int g_submit_queues = 1; -module_param_named(submit_queues, g_submit_queues, int, 0444); +NULL_PARAM(submit_queues, 1, INT_MAX); +device_param_cb(submit_queues, &null_submit_queues_param_ops, &g_submit_queues, 0444); MODULE_PARM_DESC(submit_queues, "Number of submission queues"); static int g_poll_queues = 1; -module_param_named(poll_queues, g_poll_queues, int, 0444); +NULL_PARAM(poll_queues, 1, num_online_cpus()); +device_param_cb(poll_queues, &null_poll_queues_param_ops, &g_poll_queues, 0444); MODULE_PARM_DESC(poll_queues, "Number of IOPOLL submission queues"); static int g_home_node = NUMA_NO_NODE; @@ -116,45 +145,23 @@ MODULE_PARM_DESC(init_hctx, "Fault injection to fail hctx init. init_hctx=<inter #endif static int g_queue_mode = NULL_Q_MQ; - -static int null_param_store_val(const char *str, int *val, int min, int max) -{ - int ret, new_val; - - ret = kstrtoint(str, 10, &new_val); - if (ret) - return -EINVAL; - - if (new_val < min || new_val > max) - return -EINVAL; - - *val = new_val; - return 0; -} - -static int null_set_queue_mode(const char *str, const struct kernel_param *kp) -{ - return null_param_store_val(str, &g_queue_mode, NULL_Q_BIO, NULL_Q_MQ); -} - -static const struct kernel_param_ops null_queue_mode_param_ops = { - .set = null_set_queue_mode, - .get = param_get_int, -}; - +NULL_PARAM(queue_mode, NULL_Q_BIO, NULL_Q_MQ); device_param_cb(queue_mode, &null_queue_mode_param_ops, &g_queue_mode, 0444); MODULE_PARM_DESC(queue_mode, "Block interface to use (0=bio,1=rq,2=multiqueue)"); static int g_gb = 250; -module_param_named(gb, g_gb, int, 0444); +NULL_PARAM(gb, 1, INT_MAX); +device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); MODULE_PARM_DESC(gb, "Size in GB"); static int g_bs = 512; -module_param_named(bs, g_bs, int, 0444); +NULL_PARAM(bs, 1, PAGE_SIZE); +device_param_cb(bs, &null_bs_param_ops, &g_bs, 0444); MODULE_PARM_DESC(bs, "Block size (in bytes)"); static int g_max_sectors; -module_param_named(max_sectors, g_max_sectors, int, 0444); +NULL_PARAM(max_sectors, 1, INT_MAX); +device_param_cb(max_sectors, &null_max_sectors_param_ops, &g_max_sectors, 0444); MODULE_PARM_DESC(max_sectors, "Maximum size of a command (in 512B sectors)"); static unsigned int nr_devices = 1; @@ -174,18 +181,7 @@ module_param_named(shared_tag_bitmap, g_shared_tag_bitmap, bool, 0444); MODULE_PARM_DESC(shared_tag_bitmap, "Use shared tag bitmap for all submission queues for blk-mq"); static int g_irqmode = NULL_IRQ_SOFTIRQ; - -static int null_set_irqmode(const char *str, const struct kernel_param *kp) -{ - return null_param_store_val(str, &g_irqmode, NULL_IRQ_NONE, - NULL_IRQ_TIMER); -} - -static const struct kernel_param_ops null_irqmode_param_ops = { - .set = null_set_irqmode, - .get = param_get_int, -}; - +NULL_PARAM(irqmode, NULL_IRQ_NONE, NULL_IRQ_TIMER); device_param_cb(irqmode, &null_irqmode_param_ops, &g_irqmode, 0444); MODULE_PARM_DESC(irqmode, "IRQ completion handler. 0-none, 1-softirq, 2-timer"); @@ -194,7 +190,8 @@ module_param_named(completion_nsec, g_completion_nsec, ulong, 0444); MODULE_PARM_DESC(completion_nsec, "Time in ns to complete a request in hardware. Default: 10,000ns"); static int g_hw_queue_depth = 64; -module_param_named(hw_queue_depth, g_hw_queue_depth, int, 0444); +NULL_PARAM(hw_qdepth, 1, INT_MAX); +device_param_cb(hw_queue_depth, &null_hw_qdepth_param_ops, &g_hw_queue_depth, 0444); MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 64"); static bool g_use_per_node_hctx;
Move null_param_store_int() function to the top of the code so that it can be accessed by the new NULL_PARAM() macro. The macro takes name of the module parameter, minimum value, and maximum value as input, then creates specific callback functions and kernel_param ops for each module parameter. This macro ultimately calls the null_param_store_int() function, which returns an error if the user-provided value is out of bounds(min,max). To prevent negative values from being set by the user for following list of module parameters, uses NULL_PARM() macro to add user input validation: submit_queues poll_queues queue_mode gb max_sectors irqmode hw_qdepth bs This commit improves the organization and safety of the code, making it easier to maintain and preventing users from accidentally setting negative values for the specified parameters. Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> --- drivers/block/null_blk/main.c | 85 +++++++++++++++++------------------ 1 file changed, 41 insertions(+), 44 deletions(-)