Messages in this thread | | | Date | Wed, 29 Nov 2023 12:26:24 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 1/2] nvme: Update type from size_t to u16 for opts->queue_size |
| |
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
| |