diff mbox series

misc: microchip: pci1xxxx: Fix error handling in gp_aux_bus_probe()

Message ID 20230518163333.1355445-1-harshit.m.mogalapalli@oracle.com
State New
Headers
Series misc: microchip: pci1xxxx: Fix error handling in gp_aux_bus_probe() |

Commit Message

Harshit Mogalapalli May 18, 2023, 4:33 p.m. UTC 
  Smatch warns:
	drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c:73
	gp_aux_bus_probe() warn: missing unwind goto?

Apart from above warning that smatch warns, we have other issues with
this function.

1. The call to auxiliary_device_add() needs a matching call to
   auxiliary_device_delete(). When memory allocation for
   "aux_bus->aux_device_wrapper[1]" fails we should also delete
   auxiliary device for "aux_device_wrapper[0]".
2. In the error path when auxiliary_device_uninit() is called, it
   does trigger the release function --> gp_auxiliary_device_release(),
   this release function has the following:

	ida_free(&gp_client_ida, aux_device_wrapper->aux_dev.id);
	kfree(aux_device_wrapper);

   so few error paths have double frees. Eg: The goto label
   "err_aux_dev_add_0" first calls auxiliary_device_uninit() which also
   does an ida_free(), so when the control reaches "err_aux_dev_init_0"
   it will be a double free there.

Re-write the error handling code. Clean up manually before the
auxiliary_device_init() calls succeed and use gotos to clean up after
they succeed. Also change the goto label names to follow freeing the
last thing to make it more readable.

Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for the PIO function in the multi-function endpoint of pci1xxxx device.")
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
Only compile tested, from static analysis.
---
 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 71 ++++++++++---------
 1 file changed, 38 insertions(+), 33 deletions(-)

Comments

Kumaravel Thiagarajan May 29, 2023, 9:47 a.m. UTC | #1 
> -----Original Message-----
> From: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> Sent: Thursday, May 18, 2023 10:04 PM
> 
> Smatch warns:
>         drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c:73
>         gp_aux_bus_probe() warn: missing unwind goto?
> 
> Apart from above warning that smatch warns, we have other issues with this
> function.
> 
> 1. The call to auxiliary_device_add() needs a matching call to
>    auxiliary_device_delete(). When memory allocation for
>    "aux_bus->aux_device_wrapper[1]" fails we should also delete
>    auxiliary device for "aux_device_wrapper[0]".
> 2. In the error path when auxiliary_device_uninit() is called, it
>    does trigger the release function --> gp_auxiliary_device_release(),
>    this release function has the following:
> 
>         ida_free(&gp_client_ida, aux_device_wrapper->aux_dev.id);
>         kfree(aux_device_wrapper);
> 
>    so few error paths have double frees. Eg: The goto label
>    "err_aux_dev_add_0" first calls auxiliary_device_uninit() which also
>    does an ida_free(), so when the control reaches "err_aux_dev_init_0"
>    it will be a double free there.
> 
> Re-write the error handling code. Clean up manually before the
> auxiliary_device_init() calls succeed and use gotos to clean up after they
> succeed. Also change the goto label names to follow freeing the last thing to
> make it more readable.
Thank You Harshit ! I reviewed your changes.
But I need some time to apply the changes and test them.
I will let you know as soon as we make progress.
> 
> Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for
> the PIO function in the multi-function endpoint of pci1xxxx device.")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> ---
> Only compile tested, from static analysis.

Regards,
Kumar
Kumaravel Thiagarajan June 8, 2023, 6:57 a.m. UTC | #2 
> -----Original Message-----
> From: Kumaravel Thiagarajan - I21417
> Sent: Monday, May 29, 2023 3:17 PM
> To: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> Subject: RE: [PATCH] misc: microchip: pci1xxxx: Fix error handling in
> gp_aux_bus_probe()
> 
> >
> > Smatch warns:
> >         drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c:73
> >         gp_aux_bus_probe() warn: missing unwind goto?
> >
> > Apart from above warning that smatch warns, we have other issues with
> > this function.
> >
> > 1. The call to auxiliary_device_add() needs a matching call to
> >    auxiliary_device_delete(). When memory allocation for
> >    "aux_bus->aux_device_wrapper[1]" fails we should also delete
> >    auxiliary device for "aux_device_wrapper[0]".
> > 2. In the error path when auxiliary_device_uninit() is called, it
> >    does trigger the release function --> gp_auxiliary_device_release(),
> >    this release function has the following:
> >
> >         ida_free(&gp_client_ida, aux_device_wrapper->aux_dev.id);
> >         kfree(aux_device_wrapper);
> >
> >    so few error paths have double frees. Eg: The goto label
> >    "err_aux_dev_add_0" first calls auxiliary_device_uninit() which also
> >    does an ida_free(), so when the control reaches "err_aux_dev_init_0"
> >    it will be a double free there.
> >
> > Re-write the error handling code. Clean up manually before the
> > auxiliary_device_init() calls succeed and use gotos to clean up after
> > they succeed. Also change the goto label names to follow freeing the
> > last thing to make it more readable.
> Thank You Harshit ! I reviewed your changes.
> But I need some time to apply the changes and test them.
> I will let you know as soon as we make progress.
> >
> > Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus
> > driver for the PIO function in the multi-function endpoint of pci1xxxx
> > device.")
> > Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> > ---
> > Only compile tested, from static analysis.

Jegadheesan,

Please apply and test this patch - https://lore.kernel.org/all/20230518163333.1355445-1-harshit.m.mogalapalli@oracle.com/
It would fix some bugs in the pci1xxxx'x auxiliary bus driver's error handling path in the probe function.

Thank You.

Regards,
Kumar
diff mbox series

Patch

diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
index 32af2b14ff34..f76ef6fd7bfc 100644
--- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
+++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
@@ -48,8 +48,10 @@  static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
 		return -ENOMEM;
 
 	retval = ida_alloc(&gp_client_ida, GFP_KERNEL);
-	if (retval < 0)
-		goto err_ida_alloc_0;
+	if (retval < 0) {
+		kfree(aux_bus->aux_device_wrapper[0]);
+		return retval;
+	}
 
 	aux_bus->aux_device_wrapper[0]->aux_dev.name = aux_dev_otp_e2p_name;
 	aux_bus->aux_device_wrapper[0]->aux_dev.dev.parent = &pdev->dev;
@@ -60,21 +62,28 @@  static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
 	aux_bus->aux_device_wrapper[0]->gp_aux_data.region_length = pci_resource_end(pdev, 0);
 
 	retval = auxiliary_device_init(&aux_bus->aux_device_wrapper[0]->aux_dev);
-	if (retval < 0)
-		goto err_aux_dev_init_0;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[0]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[0]);
+		return retval;
+	}
 
 	retval = auxiliary_device_add(&aux_bus->aux_device_wrapper[0]->aux_dev);
 	if (retval)
-		goto err_aux_dev_add_0;
+		goto uninit_device_wrapper_0;
 
 	aux_bus->aux_device_wrapper[1] = kzalloc(sizeof(*aux_bus->aux_device_wrapper[1]),
 						 GFP_KERNEL);
-	if (!aux_bus->aux_device_wrapper[1])
-		return -ENOMEM;
+	if (!aux_bus->aux_device_wrapper[1]) {
+		retval = -ENOMEM;
+		goto delete_device_wrapper_0;
+	}
 
 	retval = ida_alloc(&gp_client_ida, GFP_KERNEL);
-	if (retval < 0)
-		goto err_ida_alloc_1;
+	if (retval < 0) {
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	aux_bus->aux_device_wrapper[1]->aux_dev.name = aux_dev_gpio_name;
 	aux_bus->aux_device_wrapper[1]->aux_dev.dev.parent = &pdev->dev;
@@ -85,48 +94,44 @@  static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
 	aux_bus->aux_device_wrapper[1]->gp_aux_data.region_length = pci_resource_end(pdev, 0);
 
 	retval = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
-
-	if (retval < 0)
-		goto err_aux_dev_init_1;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	retval = pci_irq_vector(pdev, 0);
-	if (retval < 0)
-		goto err_aux_dev_init_1;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	pdev->irq = retval;
 	aux_bus->aux_device_wrapper[1]->gp_aux_data.irq_num = pdev->irq;
 
 	retval = auxiliary_device_init(&aux_bus->aux_device_wrapper[1]->aux_dev);
-	if (retval < 0)
-		goto err_aux_dev_init_1;
+	if (retval < 0) {
+		ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
+		kfree(aux_bus->aux_device_wrapper[1]);
+		goto delete_device_wrapper_0;
+	}
 
 	retval = auxiliary_device_add(&aux_bus->aux_device_wrapper[1]->aux_dev);
 	if (retval)
-		goto err_aux_dev_add_1;
+		goto uninit_device_wrapper_1;
 
 	pci_set_drvdata(pdev, aux_bus);
 	pci_set_master(pdev);
 
 	return 0;
 
-err_aux_dev_add_1:
+uninit_device_wrapper_1:
 	auxiliary_device_uninit(&aux_bus->aux_device_wrapper[1]->aux_dev);
-
-err_aux_dev_init_1:
-	ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
-
-err_ida_alloc_1:
-	kfree(aux_bus->aux_device_wrapper[1]);
-
-err_aux_dev_add_0:
+delete_device_wrapper_0:
+	auxiliary_device_delete(&aux_bus->aux_device_wrapper[0]->aux_dev);
+uninit_device_wrapper_0:
 	auxiliary_device_uninit(&aux_bus->aux_device_wrapper[0]->aux_dev);
-
-err_aux_dev_init_0:
-	ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[0]->aux_dev.id);
-
-err_ida_alloc_0:
-	kfree(aux_bus->aux_device_wrapper[0]);
-
 	return retval;
 }