| Message ID | 20201213114838.126922-1-christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | net: mscc: ocelot: Fix a resource leak in the error handling path of the probe function | expand |
On Sun, Dec 13, 2020 at 12:48:38PM +0100, Christophe JAILLET wrote: > In case of error after calling 'ocelot_init()', it must be undone by a > corresponding 'ocelot_deinit()' call, as already done in the remove > function. > This changes the behavior slightly in another way as well, but it's probably a bug fix. drivers/net/ethernet/mscc/ocelot_vsc7514.c 1250 ports = of_get_child_by_name(np, "ethernet-ports"); 1251 if (!ports) { 1252 dev_err(ocelot->dev, "no ethernet-ports child node found\n"); 1253 return -ENODEV; 1254 } 1255 1256 ocelot->num_phys_ports = of_get_child_count(ports); 1257 ocelot->num_flooding_pgids = 1; 1258 1259 ocelot->vcap = vsc7514_vcap_props; 1260 ocelot->inj_prefix = OCELOT_TAG_PREFIX_NONE; 1261 ocelot->xtr_prefix = OCELOT_TAG_PREFIX_NONE; 1262 ocelot->npi = -1; 1263 1264 err = ocelot_init(ocelot); 1265 if (err) 1266 goto out_put_ports; 1267 1268 err = mscc_ocelot_init_ports(pdev, ports); 1269 if (err) 1270 goto out_put_ports; 1271 1272 if (ocelot->ptp) { 1273 err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); 1274 if (err) { 1275 dev_err(ocelot->dev, 1276 "Timestamp initialization failed\n"); 1277 ocelot->ptp = 0; 1278 } In the original code, if ocelot_init_timestamp() failed we returned a negative error code but now we return success. This probably is what the original authors intended, though. 1279 } 1280 1281 register_netdevice_notifier(&ocelot_netdevice_nb); 1282 register_switchdev_notifier(&ocelot_switchdev_nb); 1283 register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb); 1284 1285 dev_info(&pdev->dev, "Ocelot switch probed\n"); 1286 1287 out_put_ports: 1288 of_node_put(ports); 1289 return err; 1290 } regards, dan carpenter
Le 14/12/2020 à 12:48, Dan Carpenter a écrit : > On Sun, Dec 13, 2020 at 12:48:38PM +0100, Christophe JAILLET wrote: >> In case of error after calling 'ocelot_init()', it must be undone by a >> corresponding 'ocelot_deinit()' call, as already done in the remove >> function. >> > > This changes the behavior slightly in another way as well, but it's > probably a bug fix. > > drivers/net/ethernet/mscc/ocelot_vsc7514.c > 1250 ports = of_get_child_by_name(np, "ethernet-ports"); > 1251 if (!ports) { > 1252 dev_err(ocelot->dev, "no ethernet-ports child node found\n"); > 1253 return -ENODEV; > 1254 } > 1255 > 1256 ocelot->num_phys_ports = of_get_child_count(ports); > 1257 ocelot->num_flooding_pgids = 1; > 1258 > 1259 ocelot->vcap = vsc7514_vcap_props; > 1260 ocelot->inj_prefix = OCELOT_TAG_PREFIX_NONE; > 1261 ocelot->xtr_prefix = OCELOT_TAG_PREFIX_NONE; > 1262 ocelot->npi = -1; > 1263 > 1264 err = ocelot_init(ocelot); > 1265 if (err) > 1266 goto out_put_ports; > 1267 > 1268 err = mscc_ocelot_init_ports(pdev, ports); > 1269 if (err) > 1270 goto out_put_ports; > 1271 > 1272 if (ocelot->ptp) { > 1273 err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); > 1274 if (err) { > 1275 dev_err(ocelot->dev, > 1276 "Timestamp initialization failed\n"); > 1277 ocelot->ptp = 0; > 1278 } > > In the original code, if ocelot_init_timestamp() failed we returned > a negative error code but now we return success. This probably is what > the original authors intended, though. > Thanks for the detailed review Dan. I agree with you. However this "fix" was not intentional. :( This may worth stating it in the commit message. Can it be done when/if the patch is applied? CJ
On 13/12/2020 12:48:38+0100, Christophe JAILLET wrote: > In case of error after calling 'ocelot_init()', it must be undone by a > corresponding 'ocelot_deinit()' call, as already done in the remove > function. > > Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > index 1e7729421a82..9cf2bc5f4289 100644 > --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c > +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c > @@ -1267,7 +1267,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev) > > err = mscc_ocelot_init_ports(pdev, ports); > if (err) > - goto out_put_ports; > + goto out_ocelot_deinit; > > if (ocelot->ptp) { > err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); > @@ -1282,8 +1282,14 @@ static int mscc_ocelot_probe(struct platform_device *pdev) > register_switchdev_notifier(&ocelot_switchdev_nb); > register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb); > > + of_node_put(ports); > + > dev_info(&pdev->dev, "Ocelot switch probed\n"); > > + return 0; > + > +out_ocelot_deinit: > + ocelot_deinit(ocelot); > out_put_ports: > of_node_put(ports); > return err; > -- > 2.27.0 >
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Sun, 13 Dec 2020 12:48:38 +0100 you wrote: > In case of error after calling 'ocelot_init()', it must be undone by a > corresponding 'ocelot_deinit()' call, as already done in the remove > function. > > Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > [...] Here is the summary with links: - net: mscc: ocelot: Fix a resource leak in the error handling path of the probe function https://git.kernel.org/netdev/net/c/f87675b836b3 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index 1e7729421a82..9cf2bc5f4289 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -1267,7 +1267,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev) err = mscc_ocelot_init_ports(pdev, ports); if (err) - goto out_put_ports; + goto out_ocelot_deinit; if (ocelot->ptp) { err = ocelot_init_timestamp(ocelot, &ocelot_ptp_clock_info); @@ -1282,8 +1282,14 @@ static int mscc_ocelot_probe(struct platform_device *pdev) register_switchdev_notifier(&ocelot_switchdev_nb); register_switchdev_blocking_notifier(&ocelot_switchdev_blocking_nb); + of_node_put(ports); + dev_info(&pdev->dev, "Ocelot switch probed\n"); + return 0; + +out_ocelot_deinit: + ocelot_deinit(ocelot); out_put_ports: of_node_put(ports); return err;
In case of error after calling 'ocelot_init()', it must be undone by a corresponding 'ocelot_deinit()' call, as already done in the remove function. Fixes: a556c76adc05 ("net: mscc: Add initial Ocelot switch support") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)