* [PATCH 0/3] VMCI: Various fixes
@ 2022-02-10 22:26 Christophe JAILLET
2022-02-10 22:27 ` [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps() Christophe JAILLET
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Christophe JAILLET @ 2022-02-10 22:26 UTC (permalink / raw)
To: jhansen, vdasa, arnd, gregkh, acking, dtor
Cc: pv-drivers, linux-kernel, kernel-janitors, Christophe JAILLET
Patch 1 and 2 should be straighforward.
But review with much more care the 3rd one, as explained below the --- in the
patch.
Christophe JAILLET (3):
VMCI: Fix the description of vmci_check_host_caps()
VMCI: No need to clear memory after a dma_alloc_coherent() call
VMCI: Fix some error handling paths in vmci_guest_probe_device()
drivers/misc/vmw_vmci/vmci_guest.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps() 2022-02-10 22:26 [PATCH 0/3] VMCI: Various fixes Christophe JAILLET @ 2022-02-10 22:27 ` Christophe JAILLET 2022-02-17 7:08 ` Vishnu Dasa 2022-02-10 22:27 ` [PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call Christophe JAILLET 2022-02-10 22:27 ` [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() Christophe JAILLET 2 siblings, 1 reply; 12+ messages in thread From: Christophe JAILLET @ 2022-02-10 22:27 UTC (permalink / raw) To: jhansen, vdasa, arnd, gregkh, acking, dtor Cc: pv-drivers, linux-kernel, kernel-janitors, Christophe JAILLET vmci_check_host_caps() doesn't return a bool but an int. Fix the description accordingly. Fixes: 782f24453536 ("VMCI: fix error handling path when registering guest driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/misc/vmw_vmci/vmci_guest.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c index aa61a687b3e2..1a1858742f75 100644 --- a/drivers/misc/vmw_vmci/vmci_guest.c +++ b/drivers/misc/vmw_vmci/vmci_guest.c @@ -253,9 +253,9 @@ static void vmci_guest_cid_update(u32 sub_id, /* * Verify that the host supports the hypercalls we need. If it does not, - * try to find fallback hypercalls and use those instead. Returns - * true if required hypercalls (or fallback hypercalls) are - * supported by the host, false otherwise. + * try to find fallback hypercalls and use those instead. Returns 0 if + * required hypercalls (or fallback hypercalls) are supported by the host, + * an error code otherwise. */ static int vmci_check_host_caps(struct pci_dev *pdev) { -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps() 2022-02-10 22:27 ` [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps() Christophe JAILLET @ 2022-02-17 7:08 ` Vishnu Dasa 0 siblings, 0 replies; 12+ messages in thread From: Vishnu Dasa @ 2022-02-17 7:08 UTC (permalink / raw) To: Christophe JAILLET Cc: Jorgen Hansen, arnd, gregkh, acking, dtor, Pv-drivers, linux-kernel, kernel-janitors > On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > vmci_check_host_caps() doesn't return a bool but an int. > Fix the description accordingly. > > Fixes: 782f24453536 ("VMCI: fix error handling path when registering guest driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/misc/vmw_vmci/vmci_guest.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c > index aa61a687b3e2..1a1858742f75 100644 > --- a/drivers/misc/vmw_vmci/vmci_guest.c > +++ b/drivers/misc/vmw_vmci/vmci_guest.c > @@ -253,9 +253,9 @@ static void vmci_guest_cid_update(u32 sub_id, > > /* > * Verify that the host supports the hypercalls we need. If it does not, > - * try to find fallback hypercalls and use those instead. Returns > - * true if required hypercalls (or fallback hypercalls) are > - * supported by the host, false otherwise. > + * try to find fallback hypercalls and use those instead. Returns 0 if > + * required hypercalls (or fallback hypercalls) are supported by the host, > + * an error code otherwise. > */ > static int vmci_check_host_caps(struct pci_dev *pdev) > { > -- > 2.32.0 > Thanks! Acked-by: Vishnu Dasa <vdasa@vmware.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call 2022-02-10 22:26 [PATCH 0/3] VMCI: Various fixes Christophe JAILLET 2022-02-10 22:27 ` [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps() Christophe JAILLET @ 2022-02-10 22:27 ` Christophe JAILLET 2022-02-17 7:10 ` Vishnu Dasa 2022-02-10 22:27 ` [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() Christophe JAILLET 2 siblings, 1 reply; 12+ messages in thread From: Christophe JAILLET @ 2022-02-10 22:27 UTC (permalink / raw) To: jhansen, vdasa, arnd, gregkh, acking, dtor Cc: pv-drivers, linux-kernel, kernel-janitors, Christophe JAILLET dma_alloc_coherent() already clear the allocated memory, there is no need to explicitly call memset(). This saves a few cycles and a few lines of code. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/misc/vmw_vmci/vmci_guest.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c index 1a1858742f75..02d4722d8474 100644 --- a/drivers/misc/vmw_vmci/vmci_guest.c +++ b/drivers/misc/vmw_vmci/vmci_guest.c @@ -706,13 +706,11 @@ static int vmci_guest_probe_device(struct pci_dev *pdev, vmci_dev->notification_bitmap = dma_alloc_coherent( &pdev->dev, PAGE_SIZE, &vmci_dev->notification_base, GFP_KERNEL); - if (!vmci_dev->notification_bitmap) { + if (!vmci_dev->notification_bitmap) dev_warn(&pdev->dev, "Unable to allocate notification bitmap\n"); - } else { - memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE); + else caps_in_use |= VMCI_CAPS_NOTIFICATIONS; - } } if (mmio_base != NULL) { -- 2.32.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call 2022-02-10 22:27 ` [PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call Christophe JAILLET @ 2022-02-17 7:10 ` Vishnu Dasa 0 siblings, 0 replies; 12+ messages in thread From: Vishnu Dasa @ 2022-02-17 7:10 UTC (permalink / raw) To: Christophe JAILLET Cc: Jorgen Hansen, arnd, gregkh, acking, dtor, Pv-drivers, linux-kernel, kernel-janitors > On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > dma_alloc_coherent() already clear the allocated memory, there is no need > to explicitly call memset(). > This saves a few cycles and a few lines of code. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/misc/vmw_vmci/vmci_guest.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/misc/vmw_vmci/vmci_guest.c b/drivers/misc/vmw_vmci/vmci_guest.c > index 1a1858742f75..02d4722d8474 100644 > --- a/drivers/misc/vmw_vmci/vmci_guest.c > +++ b/drivers/misc/vmw_vmci/vmci_guest.c > @@ -706,13 +706,11 @@ static int vmci_guest_probe_device(struct pci_dev *pdev, > vmci_dev->notification_bitmap = dma_alloc_coherent( > &pdev->dev, PAGE_SIZE, &vmci_dev->notification_base, > GFP_KERNEL); > - if (!vmci_dev->notification_bitmap) { > + if (!vmci_dev->notification_bitmap) > dev_warn(&pdev->dev, > "Unable to allocate notification bitmap\n"); > - } else { > - memset(vmci_dev->notification_bitmap, 0, PAGE_SIZE); > + else > caps_in_use |= VMCI_CAPS_NOTIFICATIONS; > - } > } > > if (mmio_base != NULL) { > -- > 2.32.0 > Thanks! Acked-by: Vishnu Dasa <vdasa@vmware.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() 2022-02-10 22:26 [PATCH 0/3] VMCI: Various fixes Christophe JAILLET 2022-02-10 22:27 ` [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps() Christophe JAILLET 2022-02-10 22:27 ` [PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call Christophe JAILLET @ 2022-02-10 22:27 ` Christophe JAILLET 2022-02-11 14:09 ` Dan Carpenter 2022-02-24 0:43 ` Vishnu Dasa 2 siblings, 2 replies; 12+ messages in thread From: Christophe JAILLET @ 2022-02-10 22:27 UTC (permalink / raw) To: jhansen, vdasa, arnd, gregkh, acking, dtor Cc: pv-drivers, linux-kernel, kernel-janitors, Christophe JAILLET 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. --- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() 2022-02-10 22:27 ` [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() Christophe JAILLET @ 2022-02-11 14:09 ` Dan Carpenter 2022-02-11 20:06 ` Christophe JAILLET 2022-02-24 0:43 ` Vishnu Dasa 1 sibling, 1 reply; 12+ messages in thread From: Dan Carpenter @ 2022-02-11 14:09 UTC (permalink / raw) To: Christophe JAILLET Cc: jhansen, vdasa, arnd, gregkh, acking, dtor, pv-drivers, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() 2022-02-11 14:09 ` Dan Carpenter @ 2022-02-11 20:06 ` Christophe JAILLET 2022-02-24 6:53 ` Vishnu Dasa 0 siblings, 1 reply; 12+ messages in thread From: Christophe JAILLET @ 2022-02-11 20:06 UTC (permalink / raw) To: Dan Carpenter Cc: jhansen, vdasa, arnd, gregkh, acking, dtor, pv-drivers, linux-kernel, kernel-janitors 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@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. 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() 2022-02-11 20:06 ` Christophe JAILLET @ 2022-02-24 6:53 ` Vishnu Dasa 2022-02-24 10:03 ` Dan Carpenter 0 siblings, 1 reply; 12+ messages in thread From: Vishnu Dasa @ 2022-02-24 6:53 UTC (permalink / raw) To: Christophe JAILLET Cc: Dan Carpenter, Jorgen Hansen, arnd, gregkh, acking, dtor, Pv-drivers, linux-kernel, kernel-janitors > On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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 <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. > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() 2022-02-24 6:53 ` Vishnu Dasa @ 2022-02-24 10:03 ` Dan Carpenter 2022-02-24 16:08 ` Vishnu Dasa 0 siblings, 1 reply; 12+ messages in thread From: Dan Carpenter @ 2022-02-24 10:03 UTC (permalink / raw) To: Vishnu Dasa Cc: Christophe JAILLET, Jorgen Hansen, arnd, gregkh, acking, dtor, Pv-drivers, linux-kernel, kernel-janitors On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote: > > > On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() 2022-02-24 10:03 ` Dan Carpenter @ 2022-02-24 16:08 ` Vishnu Dasa 0 siblings, 0 replies; 12+ messages in thread From: Vishnu Dasa @ 2022-02-24 16:08 UTC (permalink / raw) To: Dan Carpenter, Christophe JAILLET Cc: Jorgen Hansen, arnd, gregkh, dtor, Pv-drivers, linux-kernel, kernel-janitors > On Feb 24, 2022, at 2:03 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Thu, Feb 24, 2022 at 06:53:10AM +0000, Vishnu Dasa wrote: >> >>> On Feb 11, 2022, at 12:06 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() 2022-02-10 22:27 ` [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() Christophe JAILLET 2022-02-11 14:09 ` Dan Carpenter @ 2022-02-24 0:43 ` Vishnu Dasa 1 sibling, 0 replies; 12+ messages in thread From: Vishnu Dasa @ 2022-02-24 0:43 UTC (permalink / raw) To: Christophe JAILLET Cc: Jorgen Hansen, arnd, gregkh, acking, dtor, Pv-drivers, linux-kernel, kernel-janitors > On Feb 10, 2022, at 2:27 PM, Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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> Thanks for the patch! Let's deal with the points which Dan brought up separately. Acked-by: Vishnu Dasa <vdasa@vmware.com> > --- > 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-02-24 16:09 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-10 22:26 [PATCH 0/3] VMCI: Various fixes Christophe JAILLET 2022-02-10 22:27 ` [PATCH 1/3] VMCI: Fix the description of vmci_check_host_caps() Christophe JAILLET 2022-02-17 7:08 ` Vishnu Dasa 2022-02-10 22:27 ` [PATCH 2/3] VMCI: No need to clear memory after a dma_alloc_coherent() call Christophe JAILLET 2022-02-17 7:10 ` Vishnu Dasa 2022-02-10 22:27 ` [PATCH 3/3] VMCI: Fix some error handling paths in vmci_guest_probe_device() Christophe JAILLET 2022-02-11 14:09 ` Dan Carpenter 2022-02-11 20:06 ` Christophe JAILLET 2022-02-24 6:53 ` Vishnu Dasa 2022-02-24 10:03 ` Dan Carpenter 2022-02-24 16:08 ` Vishnu Dasa 2022-02-24 0:43 ` Vishnu Dasa
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).