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

[Dan Carpenter]

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 majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dan Carpenter]

Hello Upinder Malhi,

The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following Smatch
warning:

drivers/infiniband/hw/usnic/usnic_uiom.c:560
usnic_uiom_get_dev_list()
error: scheduling with locks held: 'spin_lock:lock'

drivers/infiniband/hw/usnic/usnic_uiom.c
553 struct device **usnic_uiom_get_dev_list(struct usnic_uiom_pd *pd)
554 {
555 struct usnic_uiom_dev *uiom_dev;
556 struct device **devs;
557 int i = 0;
558
559 spin_lock(&pd->lock);
560 devs = kzalloc(sizeof(*devs)*(pd->dev_cnt + 1), GFP_KERNEL);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
You can't do blocking allocations with a spin_lock held.

561 if (!devs) {
562 devs = ERR_PTR(-ENOMEM);
563 goto out;
564 }

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dan Carpenter]

This is called from qp_grp_and_vf_bind() and we are holding the
"vf->lock" so the allocation can't sleep.

Fixes: e3cf00d0a87f ('IB/usnic: Add Cisco VIC low-level hardware driver')
Signed-off-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+***@public.gmane.org>

diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
index ae6934c0d05a..16755cdab2c0 100644
--- a/drivers/infiniband/hw/usnic/usnic_uiom.c
+++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
@@ -498,7 +498,7 @@ int usnic_uiom_attach_dev_to_pd(struct usnic_uiom_pd *pd, struct device *dev)
struct usnic_uiom_dev *uiom_dev;
int err;

- uiom_dev = kzalloc(sizeof(*uiom_dev), GFP_KERNEL);
+ uiom_dev = kzalloc(sizeof(*uiom_dev), GFP_ATOMIC);
if (!uiom_dev)
return -ENOMEM;
uiom_dev->dev = dev;
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dan Carpenter]

Hello Upinder Malhi,

The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following Smatch
warning:
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c:467
usnic_ib_qp_grp_create()
error: scheduling with locks held: 'spin_lock:lock'

drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
449 BUG_ON(!spin_is_locked(&vf->lock));
^^^^^^^^^^^^^^^^^^^^^^^^
Holding lock.
Don't call BUG_ON(), use WARN_ON()?

450
451 err = usnic_vnic_res_spec_satisfied(&min_transport_spec[transport],
452 res_spec);
453 if (err) {
454 usnic_err("Spec does not meet miniumum req for transport %d\n",
455 transport);
456 log_spec(res_spec);
457 return ERR_PTR(err);
458 }
459
460 port_num = usnic_transport_rsrv_port(transport, 0);
461 if (!port_num) {
462 usnic_err("Unable to allocate port for %s\n",
463 netdev_name(ufdev->netdev));
464 return ERR_PTR(-EINVAL);
465 }
466
467 qp_grp = kzalloc(sizeof(*qp_grp), GFP_KERNEL);
^^^^^^^^^^
Sleeping allocation.

468 if (!qp_grp) {
469 usnic_err("Unable to alloc qp_grp - Out of memory\n");
470 return NULL;
471 }

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Upinder Malhi (umalhi)]

Why not?

We can switch to WARN_ON. The kernel provides this macro - assert_spin_locked - which does asserts via BUG_ON that spin_is_locked, which makes me think that using BUG_ON in conjunction with spin_is_locked is legal. We should use this macro instead of manually asserting, unless that it is not legal, in which case WARN_ON will do.

Upinder

Post by Dan Carpenter
Hello Upinder Malhi,
The patch b1819c455542: "IB/usnic: Add Cisco VIC low-level hardware
driver" from Sep 10, 2013, leads to the following Smatch
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c:467
usnic_ib_qp_grp_create()
error: scheduling with locks held: 'spin_lock:lock'
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
449 BUG_ON(!spin_is_locked(&vf->lock));
^^^^^^^^^^^^^^^^^^^^^^^^
Holding lock.
Don't call BUG_ON(), use WARN_ON()?
450
451 err = usnic_vnic_res_spec_satisfied(&min_transport_spec[transport],
452 res_spec);
453 if (err) {
454 usnic_err("Spec does not meet miniumum req for transport %d\n",
455 transport);
456 log_spec(res_spec);
457 return ERR_PTR(err);
458 }
459
460 port_num = usnic_transport_rsrv_port(transport, 0);
461 if (!port_num) {
462 usnic_err("Unable to allocate port for %s\n",
463 netdev_name(ufdev->netdev));
464 return ERR_PTR(-EINVAL);
465 }
466
467 qp_grp = kzalloc(sizeof(*qp_grp), GFP_KERNEL);
^^^^^^^^^^
Sleeping allocation.
468 if (!qp_grp) {
469 usnic_err("Unable to alloc qp_grp - Out of memory\n");
470 return NULL;
471 }
regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dan Carpenter]

Post by Upinder Malhi (umalhi)
Why not?
We can switch to WARN_ON. The kernel provides this macro -
assert_spin_locked - which does asserts via BUG_ON that
spin_is_locked, which makes me think that using BUG_ON in conjunction
with spin_is_locked is legal. We should use this macro instead of
manually asserting, unless that it is not legal, in which case WARN_ON
will do.

It's better to not halt the kernel. That way you can do some debugging.

But the main thing is the scheduling with a spin lock held. The other
I don't care about.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dan Carpenter]

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_ib_verbs.c:114
usnic_ib_fill_create_qp_resp()
warn: check that 'resp' doesn't leak information (struct has
a hole after 'transport')

drivers/infiniband/hw/usnic/usnic_ib_verbs.c
109 WARN_ON(chunk->type != USNIC_VNIC_RES_TYPE_CQ);
110 resp.cq_cnt = chunk->cnt;
111 for (i = 0; i < chunk->cnt; i++)
112 resp.cq_idx[i] = chunk->res[i]->vnic_idx;
113
114 err = ib_copy_to_udata(udata, &resp, sizeof(resp));
^^^^^
The "resp" struct has a struct hole and uninitialized struct members so
it leaks uninitialized stack information to the user (information
disclosure security bug).

115 if (err) {
116 usnic_err("Failed to copy udata for %s", us_ibdev->ib_dev.name);
117 return err;
118 }

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Upinder Malhi (umalhi)]

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

See Inline for rest.

Post by Dan Carpenter
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
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 }

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

Post by Dan Carpenter
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
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... :(

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

Post by Dan Carpenter
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 majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Dan Carpenter]

Post by Upinder Malhi (umalhi)
Hi Dan,
Can I ask what static checker you are using for these?

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

Post by Upinder Malhi (umalhi)

Post by Dan Carpenter
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 }

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

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 majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html