IB/usnic: Add Cisco VIC low-level hardware driver

Hello Upinder Malhi,

The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware driver" from Sep 10, 2013, leads to the following static checker warning:
drivers/infiniband/hw/usnic/usnic_uiom.c:47 usnic_uiom_alloc_pd()
warn: passing zero to 'PTR_ERR'"

drivers/infiniband/hw/usnic/usnic_uiom.c

   469          pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
                                      ^^^^^^^^^^^^^^^^^^^

This function returns NULL on error not error pointers.

   470          if (IS_ERR_OR_NULL(domain)) {
   471                  usnic_err("Failed to allocate IOMMU domain with err %ld\n",
   472                                  PTR_ERR(pd->domain));
   473                  kfree(pd);
   474                  return ERR_PTR(domain ? PTR_ERR(domain) : -ENOMEM);
   475          }

Similar harmless but crappy slop in:

vers/infiniband/hw/usnic/usnic_ib_main.c
249 us_ibdev = (struct usnic_ib_dev *)ib_alloc_device(sizeof(*us_ibdev));

   250          if (IS_ERR_OR_NULL(us_ibdev)) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^
   251                  usnic_err("Device %s context alloc failed\n",
   252                                  netdev_name(pci_get_drvdata(dev)));
   253                  return ERR_PTR(us_ibdev ? PTR_ERR(us_ibdev) : -EFAULT);
   254          }
   255  
   256          us_ibdev->ufdev = usnic_fwd_dev_alloc(dev);
   257          if (IS_ERR_OR_NULL(us_ibdev->ufdev)) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

258 usnic_err("Failed to alloc ufdev for %s with err %ld\n",
259 pci_name(dev), PTR_ERR(us_ibdev->ufdev));
260 goto err_dealloc;
261 }

The general confusing about what return values are leads to a bug later on:

vers/infiniband/hw/usnic/usnic_ib_main.c

   462          pf = usnic_ib_discover_pf(vf->vnic);
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This function returns ERR_PTRs. Also it has a bug and can return freed pointers. Oops... :(

   463          if (!pf) {
   464                  usnic_err("Failed to discover pf of vnic %s with err%d\n",
   465                                  pci_name(pdev), err);
   466                  goto out_clean_vnic;
   467          }
   468  
   469          vf->pf = pf;
   470          spin_lock_init(&vf->lock);
   471          mutex_lock(&pf->usdev_lock);

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Dan,
Can I ask what static checker you are using for these?

See Inline for rest.

[UM] - Unless I'm missing something, I think this is OK and preferred in case iommu_domain_alloc in future starts returning ERR_PTRs.

[UM] - Yes, that is oops indeed. Will fix.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

In this email, the released version of Smatch will only warn about the actual bug and not the others. It requires that you build the cross function database though before you test the code.

~/smatch/smatch_scripts/build_kernel_data.sh

It doesn't cause a bug, but it's unusual and considered sloppy. APIs should be clear about what error codes they return and they should almost always return either ERR_PTRs *or* NULL but not *both*. The exceptions are when NULL is a valid return which means something. For example, maybe you are looking for a file and you return NULL if the file is not found but ERR_PTR(-ENOMEM) if you have an error.

In linux if we decide that iommu_domain_alloc() should return ERR_PTRs instead of NULL pointers, then it's up to the person who decides that to change all the callers as well.

Also if you trust the lower levels to print their error messages correctly then the error handling here could be much simpler. kzalloc() for example, has very extensive error messages.

	pd->domain = domain = iommu_domain_alloc(&pci_bus_type);
	if (!domain) {
		kfree(pd);
		return -ENOMEM;
	}

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html