[PATCH 0/2] nvmet-tcp: add module parameter check

[PATCH 0/2] nvmet-tcp: add module parameter check

[Chaitanya Kulkarni]

Hi,

Add module parameter check for the nvme-tcp target to generate right
error for negative values set by user at the time of the module
loading instaed of allowing module to be loaded with those values,
that created confusion, simple testing log :-

####################################
Without this patch series 
####################################
modprobe nvmet-tcp so_priority=-2 returned 0
modprobe nvmet-tcp so_priority=-1 returned 0
modprobe nvmet-tcp so_priority=0 returned 0
modprobe nvmet-tcp so_priority=1 returned 0
modprobe nvmet-tcp so_priority=100 returned 0
modprobe nvmet-tcp idle_poll_period_usecs=-2 returned 0
modprobe nvmet-tcp idle_poll_period_usecs=-1 returned 0
modprobe nvmet-tcp idle_poll_period_usecs=0 returned 0
modprobe nvmet-tcp idle_poll_period_usecs=1 returned 0
modprobe nvmet-tcp idle_poll_period_usecs=100 returned 0

####################################
With this patch series 
####################################
Applying: nvmet-tcp: validate so_priority modparam value
Applying: nvmet-tcp: validate idle poll modparam value

modprobe: ERROR: could not insert 'nvmet_tcp': Invalid argument
modprobe nvmet-tcp so_priority=-2 returned 1
[21625.237738] nvmet_tcp: `-2' invalid for parameter `so_priority'

modprobe: ERROR: could not insert 'nvmet_tcp': Invalid argument
modprobe nvmet-tcp so_priority=-1 returned 1
[21625.268026] nvmet_tcp: `-1' invalid for parameter `so_priority'

modprobe nvmet-tcp so_priority=0 returned 0
modprobe nvmet-tcp so_priority=1 returned 0
modprobe nvmet-tcp so_priority=100 returned 0

modprobe: ERROR: could not insert 'nvmet_tcp': Invalid argument
modprobe nvmet-tcp idle_poll_period_usecs=-2 returned 1
[21625.412338] nvmet_tcp: `-2' invalid for parameter `idle_poll_period_usecs'

modprobe: ERROR: could not insert 'nvmet_tcp': Invalid argument
modprobe nvmet-tcp idle_poll_period_usecs=-1 returned 1
[21625.446106] nvmet_tcp: `-1' invalid for parameter `idle_poll_period_usecs'

modprobe nvmet-tcp idle_poll_period_usecs=0 returned 0
modprobe nvmet-tcp idle_poll_period_usecs=1 returned 0
modprobe nvmet-tcp idle_poll_period_usecs=100 returned 0

-ck

Chaitanya Kulkarni (2):
  nvmet-tcp: validate so_priority modparam value
  nvmet-tcp: validate idle poll modparam value

 drivers/nvme/target/tcp.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)



-- 
2.29.0

[PATCH 1/2] nvmet-tcp: validate so_priority modparam value

[Chaitanya Kulkarni]

The module parameter so_priority is passed to the function
sock_set_priority() which has following prototype and expect
priotity arg type to be u32:-

void sock_set_priority(struct sock *sk, u32 priority);

Add a module parameter validation callback to reject any negative
values for the so_priority as it is defigned as int. Use this
oppurtunity to update the module parameter description and print the
default value.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---

Sagi, perhaps you can suggest right upper bound for so_priority?
as I'm not sure if we TC_PRIO_MAX is a right value ?

 drivers/nvme/target/tcp.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 66e8f9fd0ca7..85ffcc8d0f99 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -20,6 +20,31 @@
 
 #define NVMET_TCP_DEF_INLINE_DATA_SIZE	(4 * PAGE_SIZE)
 
+static int 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 set_params(const char *str, const struct kernel_param *kp)
+{
+	return param_store_val(str, kp->arg, 0, INT_MAX);
+}
+
+static const struct kernel_param_ops set_param_ops = {
+	.set	= set_params,
+	.get	= param_get_int,
+};
+
 /* Define the socket priority to use for connections were it is desirable
  * that the NIC consider performing optimized packet processing or filtering.
  * A non-zero value being sufficient to indicate general consideration of any
@@ -27,8 +52,8 @@
  * values that may be unique for some NIC implementations.
  */
 static int so_priority;
-module_param(so_priority, int, 0644);
-MODULE_PARM_DESC(so_priority, "nvmet tcp socket optimize priority");
+device_param_cb(so_priority, &set_param_ops, &so_priority, 0644);
+MODULE_PARM_DESC(so_priority, "nvmet tcp socket optimize priority: Default 0");
 
 /* Define a time period (in usecs) that io_work() shall sample an activated
  * queue before determining it to be idle.  This optional module behavior
-- 
2.29.0

Re: [PATCH 1/2] nvmet-tcp: validate so_priority modparam value

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 2/2] nvmet-tcp: validate idle poll modparam value

[Chaitanya Kulkarni]

The module parameter idle_poll_period_usecs is passed to the function
usecs_to_jiffies() which has following prototype and expect
idle_poll_period_usecs arg type to be unsigned int:-

unsigned long usecs_to_jiffies(const unsigned int u);

Use similar module parameter validation callback as previous patch.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/tcp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 85ffcc8d0f99..ed98df72c76b 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -61,9 +61,10 @@ MODULE_PARM_DESC(so_priority, "nvmet tcp socket optimize priority: Default 0");
  * using advanced interrupt moderation techniques.
  */
 static int idle_poll_period_usecs;
-module_param(idle_poll_period_usecs, int, 0644);
+device_param_cb(idle_poll_period_usecs, &set_param_ops,
+		&idle_poll_period_usecs, 0644);
 MODULE_PARM_DESC(idle_poll_period_usecs,
-		"nvmet tcp io_work poll till idle time period in usecs");
+		"nvmet tcp io_work poll till idle time period in usecs: Default 0");
 
 #define NVMET_TCP_RECV_BUDGET		8
 #define NVMET_TCP_SEND_BUDGET		8
-- 
2.29.0

Re: [PATCH 2/2] nvmet-tcp: validate idle poll modparam value

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH 0/2] nvmet-tcp: add module parameter check

[Christoph Hellwig]

Thanks,

applied to nvme-6.4.