diff mbox series

[1/2] nvme: Update type from size_t to u16 for opts->queue_size

Message ID 20231128122958.2235-1-nj.shetty@samsung.com
State New
Headers
Series [1/2] nvme: Update type from size_t to u16 for opts->queue_size |

Commit Message

Nitesh Shetty Nov. 28, 2023, 12:29 p.m. UTC 
  This fixes the smatch warning, "nvme_loop_create_ctrl() warn:
'opts->queue_size - 1' 18446744073709551615 can't fit into 65535
'ctrl->ctrl.sqsize'"
We don't need size_t for queue_size, u16 should serve the purpose,
as max size is limited to NVMF_DEF_QUEUE_SIZE(1024).

Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
---
 drivers/nvme/host/fabrics.h | 2 +-
 drivers/nvme/host/fc.c      | 2 +-
 drivers/nvme/host/rdma.c    | 2 +-
 drivers/nvme/host/tcp.c     | 2 +-
 drivers/nvme/target/loop.c  | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)


base-commit: da59b416ba80e43afb2b642e0cee73739f4c6079

Comments

Sagi Grimberg Nov. 29, 2023, 9:22 a.m. UTC | #1 
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Dan Carpenter Nov. 29, 2023, 9:26 a.m. UTC | #2 
On Tue, Nov 28, 2023 at 05:59:56PM +0530, Nitesh Shetty wrote:
> This fixes the smatch warning, "nvme_loop_create_ctrl() warn:
> 'opts->queue_size - 1' 18446744073709551615 can't fit into 65535
> 'ctrl->ctrl.sqsize'"
> We don't need size_t for queue_size, u16 should serve the purpose,
> as max size is limited to NVMF_DEF_QUEUE_SIZE(1024).
> 
> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>

Huh...  I'm sorry I wasn't necessarily aware that I had published this
Smatch warning.  I feel like it has a high rate of false positives.

Generally with Smatch warnings, I'm not going to try silence every
false positive.  And I had one complaint recently that I was too focused
on silencing false positives instead of discovering new bugs...

The other thing about static analysis is that static checker developers
want 0% false positives and kernel developers want 100% false positives.
I'm a kernel developer so in code that I have looked at the rate of
false positives is very close to 100%.  Only new code has valid
warnings.

Here is what this code looks like:

drivers/nvme/target/loop.c
   573          if (opts->queue_size > ctrl->ctrl.maxcmd) {
   574                  /* warn if maxcmd is lower than queue_size */
   575                  dev_warn(ctrl->ctrl.device,
   576                          "queue_size %zu > ctrl maxcmd %u, clamping down\n",
   577                          opts->queue_size, ctrl->ctrl.maxcmd);
   578                  opts->queue_size = ctrl->ctrl.maxcmd;
   579          }
   580          ctrl->ctrl.sqsize = opts->queue_size - 1;

Smatch thinks that opts->queue_size is a value that comes from the user
in the 16-47 range.  But the bug is that Smatch thinks that
ctrl->ctrl.maxcmd is zero.  16 is greater than zero so we do the
opts->queue_size = ctrl->ctrl.maxcmd; assignment.  Then zero minus one
is ULONG_MAX so that's a very high number.

Smatch is just wrong in this case.  Let me try figure out what went
wrong.  The ctrl->ctrl.maxcmd = 0 comes from:

	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);

It's supposed to get set to unknown in nvme_loop_configure_admin_queue().
The database has the correct data.

$ smdb.py return_states nvme_loop_configure_admin_queue | grep maxcmd
drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 229 |             0|       PARAM_SET |  0 |       $->ctrl.maxcmd |             0-u16max |
drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 231 | s32min-(-1),1-s32max|       PARAM_ADD |  0 |       $->ctrl.maxcmd |             0-u16max |

But the issue is that Smatch thinks that nvme_loop_configure_admin_queue()
always fails with -12.  The reason for that is because Smatch thinks
that ctrl->ctrl.ops is NULL but the function can only succeed when it's
non-NULL.

The ctrl->ops assignment happens in nvme_init_ctrl() and it should have
been easy to track.  I am not sure what went wrong there.  I'll take a
look at that and fix it.

regards,
dan carpenter
Nitesh Shetty Nov. 29, 2023, 10:48 a.m. UTC | #3 
On 29/11/23 12:26PM, Dan Carpenter wrote:
>On Tue, Nov 28, 2023 at 05:59:56PM +0530, Nitesh Shetty wrote:
>> This fixes the smatch warning, "nvme_loop_create_ctrl() warn:
>> 'opts->queue_size - 1' 18446744073709551615 can't fit into 65535
>> 'ctrl->ctrl.sqsize'"
>> We don't need size_t for queue_size, u16 should serve the purpose,
>> as max size is limited to NVMF_DEF_QUEUE_SIZE(1024).
>>
>> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>
>
>Huh...  I'm sorry I wasn't necessarily aware that I had published this
>Smatch warning.  I feel like it has a high rate of false positives.
>
>Generally with Smatch warnings, I'm not going to try silence every
>false positive.  And I had one complaint recently that I was too focused
>on silencing false positives instead of discovering new bugs...
>
>The other thing about static analysis is that static checker developers
>want 0% false positives and kernel developers want 100% false positives.
>I'm a kernel developer so in code that I have looked at the rate of
>false positives is very close to 100%.  Only new code has valid
>warnings.
>
>Here is what this code looks like:
>
>drivers/nvme/target/loop.c
>   573          if (opts->queue_size > ctrl->ctrl.maxcmd) {
>   574                  /* warn if maxcmd is lower than queue_size */
>   575                  dev_warn(ctrl->ctrl.device,
>   576                          "queue_size %zu > ctrl maxcmd %u, clamping down\n",
>   577                          opts->queue_size, ctrl->ctrl.maxcmd);
>   578                  opts->queue_size = ctrl->ctrl.maxcmd;
>   579          }
>   580          ctrl->ctrl.sqsize = opts->queue_size - 1;
>
>Smatch thinks that opts->queue_size is a value that comes from the user
>in the 16-47 range.  But the bug is that Smatch thinks that
>ctrl->ctrl.maxcmd is zero.  16 is greater than zero so we do the
>opts->queue_size = ctrl->ctrl.maxcmd; assignment.  Then zero minus one
>is ULONG_MAX so that's a very high number.
>
>Smatch is just wrong in this case.  Let me try figure out what went
>wrong.  The ctrl->ctrl.maxcmd = 0 comes from:
>
>	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
>
>It's supposed to get set to unknown in nvme_loop_configure_admin_queue().
>The database has the correct data.
>
>$ smdb.py return_states nvme_loop_configure_admin_queue | grep maxcmd
>drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 229 |             0|       PARAM_SET |  0 |       $->ctrl.maxcmd |             0-u16max |
>drivers/nvme/target/loop.c | nvme_loop_configure_admin_queue | 231 | s32min-(-1),1-s32max|       PARAM_ADD |  0 |       $->ctrl.maxcmd |             0-u16max |
>
>But the issue is that Smatch thinks that nvme_loop_configure_admin_queue()
>always fails with -12.  The reason for that is because Smatch thinks
>that ctrl->ctrl.ops is NULL but the function can only succeed when it's
>non-NULL.
>
>The ctrl->ops assignment happens in nvme_init_ctrl() and it should have
>been easy to track.  I am not sure what went wrong there.  I'll take a
>look at that and fix it.

Thank you for this insight.
I ran smatch on complete kernel using smatch's test_kernel.sh
I was unaware of this smbd.py option. I will explore this.
Meanwhile I feel this patch is still relevant, as it aligns opts
queue_size with size of ctrl queue_size.

Regards,
Nitesh Shetty
Dan Carpenter Nov. 29, 2023, 12:15 p.m. UTC | #4 
On Wed, Nov 29, 2023 at 12:26:24PM +0300, Dan Carpenter wrote:
> The ctrl->ops assignment happens in nvme_init_ctrl() and it should have
> been easy to track.  I am not sure what went wrong there.  I'll take a
> look at that and fix it.

I suspect on your system you don't have the cross function DB built so
it's not aware what happens in nvme_init_ctrl() or the other function I
mentioned.

On my system there is too much data so it isn't parsed.
1) When there is too much data it should just mark ctrl->ops as unknown
2) I should try to consolidate the data.  I should just mark all of
   ctrl->ctrl_device as unknown instead of recording any of the struct
   members individually.  There might also be stuff that isn't used like
   ctrl->namespaces_rwsem internals.
3) On my system, I have 2187 bogus entries that say we removed an item
   from a list.

Probably any of these fixes would silence the false positive but it
would be better to do all three.

If you don't have the cross function database though, you're probably
out of luck.  There are always going to be more false positives if you
don't have the cross function information.

regards,
dan carpenter
Dan Carpenter Nov. 30, 2023, 7:07 a.m. UTC | #5 
On Wed, Nov 29, 2023 at 04:18:37PM +0530, Nitesh Shetty wrote:
> Thank you for this insight.
> I ran smatch on complete kernel using smatch's test_kernel.sh
> I was unaware of this smbd.py option. I will explore this.

The ./smatch_scripts/build_kernel_data.sh command creates the cross
function db.  It takes a while though.  And it's probably better to
run it a few times because every time you run it the call tree chains
get one call longer.  I run it every night against the latest
linux-next.

> Meanwhile I feel this patch is still relevant, as it aligns opts
> queue_size with size of ctrl queue_size.

I don't have an opinion on this.

regards,
dan carpenter
Nitesh Shetty Nov. 30, 2023, 9:57 a.m. UTC | #6 
On 30/11/23 10:07AM, Dan Carpenter wrote:
>On Wed, Nov 29, 2023 at 04:18:37PM +0530, Nitesh Shetty wrote:
>> Thank you for this insight.
>> I ran smatch on complete kernel using smatch's test_kernel.sh
>> I was unaware of this smbd.py option. I will explore this.
>
>The ./smatch_scripts/build_kernel_data.sh command creates the cross
>function db.  It takes a while though.  And it's probably better to
>run it a few times because every time you run it the call tree chains
>get one call longer.  I run it every night against the latest
>linux-next.
>
I was not aware of repeated runs and chaining, will use repeat runs next time.

Thank you,
Nitesh Shetty
diff mbox series

Patch

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index fbaee5a7be19..0b2e45faa3b9 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -125,7 +125,7 @@  struct nvmf_ctrl_options {
 	char			*trsvcid;
 	char			*host_traddr;
 	char			*host_iface;
-	size_t			queue_size;
+	u16			queue_size;
 	unsigned int		nr_io_queues;
 	unsigned int		reconnect_delay;
 	bool			discovery_nqn;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 9f9a3b35dc64..7f680443b3cf 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3159,7 +3159,7 @@  nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	if (opts->queue_size > ctrl->ctrl.maxcmd) {
 		/* warn if maxcmd is lower than queue_size */
 		dev_warn(ctrl->ctrl.device,
-			"queue_size %zu > ctrl maxcmd %u, reducing "
+			"queue_size %u > ctrl maxcmd %u, reducing "
 			"to maxcmd\n",
 			opts->queue_size, ctrl->ctrl.maxcmd);
 		opts->queue_size = ctrl->ctrl.maxcmd;
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 6d178d555920..d0b7543f753e 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1025,7 +1025,7 @@  static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 
 	if (ctrl->ctrl.opts->queue_size > ctrl->ctrl.sqsize + 1) {
 		dev_warn(ctrl->ctrl.device,
-			"queue_size %zu > ctrl sqsize %u, clamping down\n",
+			"queue_size %u > ctrl sqsize %u, clamping down\n",
 			ctrl->ctrl.opts->queue_size, ctrl->ctrl.sqsize + 1);
 	}
 
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index d79811cfa0ce..d03f5921d344 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2193,7 +2193,7 @@  static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 
 	if (opts->queue_size > ctrl->sqsize + 1)
 		dev_warn(ctrl->device,
-			"queue_size %zu > ctrl sqsize %u, clamping down\n",
+			"queue_size %u > ctrl sqsize %u, clamping down\n",
 			opts->queue_size, ctrl->sqsize + 1);
 
 	if (ctrl->sqsize + 1 > ctrl->maxcmd) {
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 9cb434c58075..0416f41fad19 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -573,7 +573,7 @@  static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 	if (opts->queue_size > ctrl->ctrl.maxcmd) {
 		/* warn if maxcmd is lower than queue_size */
 		dev_warn(ctrl->ctrl.device,
-			"queue_size %zu > ctrl maxcmd %u, clamping down\n",
+			"queue_size %u > ctrl maxcmd %u, clamping down\n",
 			opts->queue_size, ctrl->ctrl.maxcmd);
 		opts->queue_size = ctrl->ctrl.maxcmd;
 	}