| Message ID | 20230518163333.1355445-1-harshit.m.mogalapalli@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | misc: microchip: pci1xxxx: Fix error handling in gp_aux_bus_probe() | expand |
> -----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
> -----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 --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; }
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(-)