| Message ID | 20230330213134.131298-6-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: add modparam checks | expand |
On 3/31/23 06:31, Chaitanya Kulkarni wrote: > Right now we don't check for valid module parameter value for > block size, that allows user to set negative values. > > Add a callback to error out when block size value is set < 1 before > module is loaded. > > Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/block/null_blk/main.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index f55d88ebd7e6..d8d79c66a7aa 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -190,8 +190,23 @@ static int g_gb = 250; > device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); > MODULE_PARM_DESC(gb, "Size in GB"); > > +static int null_set_bs(const char *s, const struct kernel_param *kp) > +{ > + int ret; > + > + ret = null_param_store_int(s, kp->arg, 512, INT_MAX); > + if (ret) > + pr_err("valid range for bs value [512 ... %d]\n", INT_MAX); This is is only checking the range. block sizes must be power-of-2 as well but that is not checked. And for the range, block size up to INT_MAX ? That is not very reasonable. > + return ret; > +} > + > +static const struct kernel_param_ops null_bs_param_ops = { > + .set = null_set_bs, > + .get = param_get_int, > +}; > + > static int g_bs = 512; > -module_param_named(bs, g_bs, int, 0444); > +device_param_cb(bs, &null_bs_param_ops, &g_bs, 0444); > MODULE_PARM_DESC(bs, "Block size (in bytes)"); > > static int g_max_sectors;
On 3/30/23 15:45, Damien Le Moal wrote: > On 3/31/23 06:31, Chaitanya Kulkarni wrote: >> Right now we don't check for valid module parameter value for >> block size, that allows user to set negative values. >> >> Add a callback to error out when block size value is set < 1 before >> module is loaded. >> >> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >> --- >> drivers/block/null_blk/main.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >> index f55d88ebd7e6..d8d79c66a7aa 100644 >> --- a/drivers/block/null_blk/main.c >> +++ b/drivers/block/null_blk/main.c >> @@ -190,8 +190,23 @@ static int g_gb = 250; >> device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); >> MODULE_PARM_DESC(gb, "Size in GB"); >> >> +static int null_set_bs(const char *s, const struct kernel_param *kp) >> +{ >> + int ret; >> + >> + ret = null_param_store_int(s, kp->arg, 512, INT_MAX); >> + if (ret) >> + pr_err("valid range for bs value [512 ... %d]\n", INT_MAX); > This is is only checking the range. block sizes must be power-of-2 as well but > that is not checked. And for the range, block size up to INT_MAX ? That is not > very reasonable. > > I'll add ^2 check to next version. any suggestions on what is a reasonable size we should limit to ? -ck
On 3/31/23 07:52, Chaitanya Kulkarni wrote: > On 3/30/23 15:45, Damien Le Moal wrote: >> On 3/31/23 06:31, Chaitanya Kulkarni wrote: >>> Right now we don't check for valid module parameter value for >>> block size, that allows user to set negative values. >>> >>> Add a callback to error out when block size value is set < 1 before >>> module is loaded. >>> >>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >>> --- >>> drivers/block/null_blk/main.c | 17 ++++++++++++++++- >>> 1 file changed, 16 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >>> index f55d88ebd7e6..d8d79c66a7aa 100644 >>> --- a/drivers/block/null_blk/main.c >>> +++ b/drivers/block/null_blk/main.c >>> @@ -190,8 +190,23 @@ static int g_gb = 250; >>> device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); >>> MODULE_PARM_DESC(gb, "Size in GB"); >>> >>> +static int null_set_bs(const char *s, const struct kernel_param *kp) >>> +{ >>> + int ret; >>> + >>> + ret = null_param_store_int(s, kp->arg, 512, INT_MAX); >>> + if (ret) >>> + pr_err("valid range for bs value [512 ... %d]\n", INT_MAX); >> This is is only checking the range. block sizes must be power-of-2 as well but >> that is not checked. And for the range, block size up to INT_MAX ? That is not >> very reasonable. >> >> > > I'll add ^2 check to next version. > any suggestions on what is a reasonable size we should limit to ? 4K if you want to limit this to testing only things that actually exist. 64K sound like a reasonable limit to test things that could eventually (maybe) appear. But allowing > 4K block size may actually trigger a lot of bugs in other code (FS). So I wonder if it is really wise to allow that.
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index f55d88ebd7e6..d8d79c66a7aa 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -190,8 +190,23 @@ static int g_gb = 250; device_param_cb(gb, &null_gb_param_ops, &g_gb, 0444); MODULE_PARM_DESC(gb, "Size in GB"); +static int null_set_bs(const char *s, const struct kernel_param *kp) +{ + int ret; + + ret = null_param_store_int(s, kp->arg, 512, INT_MAX); + if (ret) + pr_err("valid range for bs value [512 ... %d]\n", INT_MAX); + return ret; +} + +static const struct kernel_param_ops null_bs_param_ops = { + .set = null_set_bs, + .get = param_get_int, +}; + static int g_bs = 512; -module_param_named(bs, g_bs, int, 0444); +device_param_cb(bs, &null_bs_param_ops, &g_bs, 0444); MODULE_PARM_DESC(bs, "Block size (in bytes)"); static int g_max_sectors;
Right now we don't check for valid module parameter value for block size, that allows user to set negative values. Add a callback to error out when block size value is set < 1 before module is loaded. Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> --- drivers/block/null_blk/main.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)