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 <[email protected]>
---
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.
---
drivers/misc/vmw_vmci/vmci_guest.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
index 02d4722d8474..2e8ad36895ae 100644
--- a/drivers/misc/vmw_vmci/vmci_guest.c
+++ b/drivers/misc/vmw_vmci/vmci_guest.c
@@ -765,7 +765,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
/* Check host capabilities. */
error = vmci_check_host_caps(pdev);
if (error)
- goto err_remove_bitmap;
+ goto err_remove_vmci_dev_g;
/* Enable device. */
@@ -795,7 +795,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
error = pci_alloc_irq_vectors(pdev, 1, 1,
PCI_IRQ_MSIX | PCI_IRQ_MSI | PCI_IRQ_LEGACY);
if (error < 0)
- goto err_remove_bitmap;
+ goto err_unsubscrive_event;
} else {
vmci_dev->exclusive_vectors = true;
}
@@ -871,13 +871,19 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
err_disable_msi:
pci_free_irq_vectors(pdev);
+err_unsubscrive_event:
vmci_err = vmci_event_unsubscribe(ctx_update_sub_id);
if (vmci_err < VMCI_SUCCESS)
dev_warn(&pdev->dev,
"Failed to unsubscribe from event (type=%d) with subscriber (ID=0x%x): %d\n",
VMCI_EVENT_CTX_ID_UPDATE, ctx_update_sub_id, vmci_err);
-err_remove_bitmap:
+err_remove_vmci_dev_g:
+ spin_lock_irq(&vmci_dev_spinlock);
+ vmci_pdev = NULL;
+ vmci_dev_g = NULL;
+ spin_unlock_irq(&vmci_dev_spinlock);
+
if (vmci_dev->notification_bitmap) {
vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
dma_free_coherent(&pdev->dev, PAGE_SIZE,
@@ -885,12 +891,6 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
vmci_dev->notification_base);
}
-err_remove_vmci_dev_g:
- spin_lock_irq(&vmci_dev_spinlock);
- vmci_pdev = NULL;
- vmci_dev_g = NULL;
- spin_unlock_irq(&vmci_dev_spinlock);
-
err_free_data_buffers:
vmci_free_dg_buffers(vmci_dev);
--
2.32.0
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
> On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <[email protected]> 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 <[email protected]>
Thanks for the patch!
Let's deal with the points which Dan brought up separately.
Acked-by: Vishnu Dasa <[email protected]>
> ---
> 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.
> ---
> drivers/misc/vmw_vmci/vmci_guest.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c
> index 02d4722d8474..2e8ad36895ae 100644
> --- a/drivers/misc/vmw_vmci/vmci_guest.c
> +++ b/drivers/misc/vmw_vmci/vmci_guest.c
> @@ -765,7 +765,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> /* Check host capabilities. */
> error = vmci_check_host_caps(pdev);
> if (error)
> - goto err_remove_bitmap;
> + goto err_remove_vmci_dev_g;
>
> /* Enable device. */
>
> @@ -795,7 +795,7 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> error = pci_alloc_irq_vectors(pdev, 1, 1,
> PCI_IRQ_MSIX | PCI_IRQ_MSI | PCI_IRQ_LEGACY);
> if (error < 0)
> - goto err_remove_bitmap;
> + goto err_unsubscrive_event;
> } else {
> vmci_dev->exclusive_vectors = true;
> }
> @@ -871,13 +871,19 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> err_disable_msi:
> pci_free_irq_vectors(pdev);
>
> +err_unsubscrive_event:
Please rename this to err_unsubscribe_event
> vmci_err = vmci_event_unsubscribe(ctx_update_sub_id);
> if (vmci_err < VMCI_SUCCESS)
> dev_warn(&pdev->dev,
> "Failed to unsubscribe from event (type=%d) with subscriber (ID=0x%x): %d\n",
> VMCI_EVENT_CTX_ID_UPDATE, ctx_update_sub_id, vmci_err);
>
> -err_remove_bitmap:
> +err_remove_vmci_dev_g:
> + spin_lock_irq(&vmci_dev_spinlock);
> + vmci_pdev = NULL;
> + vmci_dev_g = NULL;
> + spin_unlock_irq(&vmci_dev_spinlock);
> +
> if (vmci_dev->notification_bitmap) {
> vmci_write_reg(vmci_dev, VMCI_CONTROL_RESET, VMCI_CONTROL_ADDR);
> dma_free_coherent(&pdev->dev, PAGE_SIZE,
> @@ -885,12 +891,6 @@ static int vmci_guest_probe_device(struct pci_dev *pdev,
> vmci_dev->notification_base);
> }
>
> -err_remove_vmci_dev_g:
> - spin_lock_irq(&vmci_dev_spinlock);
> - vmci_pdev = NULL;
> - vmci_dev_g = NULL;
> - spin_unlock_irq(&vmci_dev_spinlock);
> -
> err_free_data_buffers:
> vmci_free_dg_buffers(vmci_dev);
>
> --
> 2.32.0
>
> On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <[email protected]> wrote:
>
> 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 <[email protected]>
>>> ---
>>> 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.
Thanks, Christophe and Dan!
>
>> 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.
Good catch. This fixes recent code, so a separate patch would be good.
"[PATCH v3 3/8] VMCI: dma dg: detect DMA datagram capability"
>
>> 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'.
>
Right. And we would need to check 'capabilities & VMCI_CAPS_NOTIFICATIONS',
'caps_in_use & VMCI_CAPS_NOTIFICATIONS' and then
'vmci_dev->notification_bitmap' if we go that route. I think we can leave it as is.
>
>> 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.
Sorry, I hope I'm not missing something obvious or misunderstanding the point.
But I don't think the problem implied here exists?
If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
is no free_irq in this path. If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'. Note that
this is for the previous one without the check for vmci_dev->exclusive_vectors.
>
>> 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
On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
>
> > On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <[email protected]> wrote:
> >
> > Le 11/02/2022 ? 15:09, Dan Carpenter a ?crit :
> >> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
> >> 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.
>
> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
> But I don't think the problem implied here exists?
>
> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
> is no free_irq in this path. If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'. Note that
> this is for the previous one without the check for vmci_dev->exclusive_vectors.
It's not a bug, but it's bad style.
Ideally the allocation code and the free code should mirror each other
as much as possible. If there is an if statement in the allocation code
we should copy that same if statement into the free code. Even if we
can leave the if statement out, we should still include it for
readability.
Also if we add another allocation at the end of the function then we
will have to add the if statement back. Maybe the person who adds the
if statement will forget. So it's fragile.
regards,
dan carpenter
> On Feb 24, 2022, at 2:03 AM, Dan Carpenter <[email protected]> wrote:
>
> On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote:
>>
>>> On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <[email protected]> wrote:
>>>
>>> Le 11/02/2022 à 15:09, Dan Carpenter a écrit :
>>>> On Thu, Feb 10, 2022 at 11:27:34PM +0100, Christophe JAILLET wrote:
>
>>>> 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.
>>
>> Sorry, I hope I'm not missing something obvious or misunderstanding the point.
>> But I don't think the problem implied here exists?
>>
>> If 'request_irq(pci_irq_vector(pdev, 0), ...' fails we goto err_disable_msi and there
>> is no free_irq in this path. If 'request_irq(pci_irq_vector(pdev, 1), ...' fails then we
>> goto err_free_irq and we do 'free_irq(pci_irq_vector(pdev, 0), vmci_dev)'. Note that
>> this is for the previous one without the check for vmci_dev->exclusive_vectors.
>
> It's not a bug, but it's bad style.
>
> Ideally the allocation code and the free code should mirror each other
> as much as possible. If there is an if statement in the allocation code
> we should copy that same if statement into the free code. Even if we
> can leave the if statement out, we should still include it for
> readability.
>
> Also if we add another allocation at the end of the function then we
> will have to add the if statement back. Maybe the person who adds the
> if statement will forget. So it's fragile.
No disagreements about that. Making sure I wasn't missing anything
else.
Could we have a separate fix for this which fixes?
"VMCI: dma dg: register dummy IRQ handlers for DMA datagrams"
Thanks!