Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device()

[Christophe JAILLET]

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