On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
> The 'err_remove_vmci_dev_g' error label is not at the right place.
> This could lead to un-released resource.
>
> There is also a missing label. If pci_alloc_irq_vectors() fails, the
> previous vmci_event_subscribe() call must be undone.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> ---
> Review with GREAT care.
>
> This patch is a recent rebase of an old patch that has never been
> submitted.
> This function is huge and modifying its error handling path is error
> prone (at least for me).
>
> The patch is compile-tested only.
There is still one bug. Sorry if the line numbers are off.
drivers/misc/vmw_vmci/vmci_guest.c
705 if (capabilities & VMCI_CAPS_NOTIFICATIONS) {
706 vmci_dev->notification_bitmap = dma_alloc_coherent(
^^^^^
Alloc
707 &pdev->dev, PAGE_SIZE, &vmci_dev->notification_base,
708 GFP_KERNEL);
709 if (!vmci_dev->notification_bitmap) {
710 dev_warn(&pdev->dev,
711 "Unable to allocate notification bitmap\n");
712 } else {
713 memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE);
714 caps_in_use |= VMCI_CAPS_NOTIFICATIONS;
715 }
716 }
717
718 if (mmio_base != NULL) {
719 if (capabilities & VMCI_CAPS_DMA_DATAGRAM) {
720 caps_in_use |= VMCI_CAPS_DMA_DATAGRAM;
721 } else {
722 dev_err(&pdev->dev,
723 "Missing capability: VMCI_CAPS_DMA_DATAGRAM\n");
724 error = -ENXIO;
725 goto err_free_data_buffers;
This should be goto err_free_notification_bitmap;
726 }
727 }
On of the rules for error handling is that the unwind code should mirror
the allocation code but instead of that this code will have:
Alloc:
if (capabilities & VMCI_CAPS_NOTIFICATIONS)
Free:
if (vmci_dev->notification_bitmap)
It's the same if statement but you wouldn't really know it from just
looking at it so it's confusing. Whatever... But where this really
hurts is with:
Alloc:
if (vmci_dev->exclusive_vectors) {
error = request_irq(pci_irq_vector(pdev, 1), ...
Free:
free_irq(pci_irq_vector(pdev, 1), vmci_dev);
No if statement. It works because it's the last allocation but it's
confusing and fragile.
The other question I had was:
882 err_remove_bitmap:
883 if (vmci_dev->notification_bitmap) {
884 vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This doesn't mirror anything in the allocation code so who knows if its
done in the correct place/order.
885 dma_free_coherent(&pdev->dev, PAGE_SIZE,
886 vmci_dev->notification_bitmap,
887 vmci_dev->notification_base);
888 }
regards,
dan carpenter
