On Wed, Dec 11, 2024 at 06:03:04PM -0800, Karan Tilak Kumar wrote:
> @@ -612,6 +615,7 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> unsigned long flags;
> int hwq;
> char *desc, *subsys_desc;
> + int len;
Do not introduce unnecessary levels of indirection. Get rid of this len
variable.
>
> /*
> * Allocate SCSI Host and set up association between host,
> @@ -646,9 +650,17 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> fnic_stats_debugfs_init(fnic);
>
> /* Find model name from PCIe subsys ID */
> - if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0)
> + if (fnic_get_desc_by_devid(pdev, &desc, &subsys_desc) == 0) {
> dev_info(&fnic->pdev->dev, "Model: %s\n", subsys_desc);
> - else {
> +
> + /* Update FDMI model */
This comment adds no information. Delete it.
> + fnic->subsys_desc_len = strlen(subsys_desc);
Keep in mind that strlen() does not count the NUL-terminator.
> + len = ARRAY_SIZE(fnic->subsys_desc);
Use sizeof() when you are talking about bytes or chars. For snprintf() and
other string functions, it's always sizeof() and never ARRAY_SIZE().
> + if (fnic->subsys_desc_len > len)
> + fnic->subsys_desc_len = len;
> + memcpy(fnic->subsys_desc, subsys_desc, fnic->subsys_desc_len);
So this is an 0-14 character buffer. If fnic->subsys_desc_len is set to 14,
then the string is not NUL terminated. This is how the buffer is used in
fdls_fdmi_register_hba()
strscpy_pad(data, fnic->subsys_desc, FNIC_FDMI_MODEL_LEN);
data[FNIC_FDMI_MODEL_LEN - 1] = 0;
This suggests that fnic->subsys_desc is expected to be NUL-terminated.
However FNIC_FDMI_MODEL_LEN is 12. So in that case the last 3 characters
are removed. LOL. It's harmless but so very annoying.
Also strscpy_pad() will ensure that data[FNIC_FDMI_MODEL_LEN - 1] is set
to zero so that line could be deleted.
regards,
dan carpenter
