Messages in this thread | | | Date | Fri, 11 Feb 2022 17:09:50 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() |
| |
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@wanadoo.fr> > --- > 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
| |