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 <[email protected]>
---
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(-)
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;
}
base-commit: da59b416ba80e43afb2b642e0cee73739f4c6079
--
2.35.1.500.gb896f729e2
Reviewed-by: Sagi Grimberg <[email protected]>
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 <[email protected]>
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
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
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