Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
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.
Thanks for the review Dan.
Much appreciated.
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;
Agreed.
The error handling path still looked odd to me because 2 things were
undone without a label between the 2 steps.
That was it. An err_free_notification_bitmap should be added and used.
I missed it.
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.
This one is fine I think. If the allocation of notification_bitmap
fails, it is not an error. So it looks fine to test it the way it is done.
Or we should have both 'if'.
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.
Agreed.
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.
Agreed. It puzzled me as well.
vmci_guest_remove_device() also has this kind of code, but it is not
done the same way in this function. It is unconditional and not close to
the dma_free_coherent() call.
Odd.
I won't touch it by myself :)
885 dma_free_coherent(&pdev->dev, PAGE_SIZE,
886 vmci_dev->notification_bitmap,
887 vmci_dev->notification_base);
888 }
regards,
dan carpenter
All your comments are unrelated to my patch and looks like additional fixes.
Until recently, this file was mostly untouched.
So, let see if a maintainer looks interested in these patches and if he
prefers a patch that fixes everything or several patches, maybe easier
to review.
Once again, big thanks.
CJ
