Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Wed, 29 Nov 2023 16:18:37 +0530
From:   Nitesh Shetty <nj.shetty@samsung.com>
To:     Dan Carpenter <dan.carpenter@linaro.org>
Cc:     Keith Busch <kbusch@kernel.org>, Jens Axboe <axboe@kernel.dk>,
        Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        James Smart <james.smart@broadcom.com>,
        Chaitanya Kulkarni <kch@nvidia.com>, error27@gmail.com,
        gost.dev@samsung.com, nitheshshetty@gmail.com,
        linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] nvme: Update type from size_t to u16 for
 opts->queue_size
Message-ID: <20231129104837.arls2gn3wttiqiff@green245>
MIME-Version: 1.0
In-Reply-To: <0cd6862e-8037-40b3-9fde-b7f10d66e31c@suswa.mountain>
User-Agent: NeoMutt/20171215
Content-Type: multipart/mixed;
        boundary="----enUWWt6WoF-Ggb2-rjyEux7skZ5dSWJ5iC_e377tM680ja-K=_1a776_"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <CGME20231128123622epcas5p4940679fbbafdf0da802deea3e531f850@epcas5p4.samsung.com>
        <20231128122958.2235-1-nj.shetty@samsung.com>
        <0cd6862e-8037-40b3-9fde-b7f10d66e31c@suswa.mountain>
Precedence: bulk

------enUWWt6WoF-Ggb2-rjyEux7skZ5dSWJ5iC_e377tM680ja-K=_1a776_
Content-Type: text/plain; charset="utf-8"; format="flowed"
Content-Disposition: inline

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

------enUWWt6WoF-Ggb2-rjyEux7skZ5dSWJ5iC_e377tM680ja-K=_1a776_
Content-Type: text/plain; charset="utf-8"


------enUWWt6WoF-Ggb2-rjyEux7skZ5dSWJ5iC_e377tM680ja-K=_1a776_--