| Message ID | 20241015060122.25709-3-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1dec67e0d9fbb087c2ab17bf1bd17208231c3bb1 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | fsl/fman: Fix refcount handling of fman-related devices | expand |
On 10/15/24 08:01, Aleksandr Mishin wrote: > In mac_probe() there are multiple calls to of_find_device_by_node(), > fman_bind() and fman_port_bind() which takes references to of_dev->dev. > Not all references taken by these calls are released later on error path > in mac_probe() and in mac_remove() which lead to reference leaks. > > Add references release. > > Fixes: 3933961682a3 ("fsl/fman: Add FMan MAC driver") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > Compile tested only. > > drivers/net/ethernet/freescale/fman/mac.c | 62 +++++++++++++++++------ > 1 file changed, 47 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c > index 9b863db0bf08..11da139082e1 100644 > --- a/drivers/net/ethernet/freescale/fman/mac.c > +++ b/drivers/net/ethernet/freescale/fman/mac.c > @@ -204,7 +204,7 @@ static int mac_probe(struct platform_device *_of_dev) > if (err) { > dev_err(dev, "failed to read cell-index for %pOF\n", dev_node); > err = -EINVAL; > - goto _return_of_node_put; > + goto _return_dev_put; We are after a succesful of_find_device_by_node and prior to fman_bind(), mac_dev->fman_dev refcount is 1 > @@ -213,40 +213,51 @@ static int mac_probe(struct platform_device *_of_dev) > if (!priv->fman) { > dev_err(dev, "fman_bind(%pOF) failed\n", dev_node); > err = -ENODEV; > - goto _return_of_node_put; > + goto _return_dev_put; > } > > + /* Two references have been taken in of_find_device_by_node() > + * and fman_bind(). Release one of them here. The second one > + * will be released in mac_remove(). > + */ > + put_device(mac_dev->fman_dev); > of_node_put(dev_node); > + dev_node = NULL; > > /* Get the address of the memory mapped registers */ > mac_dev->res = platform_get_mem_or_io(_of_dev, 0); > if (!mac_dev->res) { > dev_err(dev, "could not get registers\n"); > - return -EINVAL; > + err = -EINVAL; > + goto _return_dev_put; Here we are after a successful fman_bind(), mac_dev->fman_dev refcount is 2. _return_dev_put will drop a single reference, this error path looks buggy. Similar issue for the _return_dev_arr_put error path below. Cheers, Paolo
On 17.10.2024 13:01, Paolo Abeni wrote: > On 10/15/24 08:01, Aleksandr Mishin wrote: >> In mac_probe() there are multiple calls to of_find_device_by_node(), >> fman_bind() and fman_port_bind() which takes references to of_dev->dev. >> Not all references taken by these calls are released later on error path >> in mac_probe() and in mac_remove() which lead to reference leaks. >> >> Add references release. >> >> Fixes: 3933961682a3 ("fsl/fman: Add FMan MAC driver") >> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> >> --- >> Compile tested only. >> >> drivers/net/ethernet/freescale/fman/mac.c | 62 +++++++++++++++++------ >> 1 file changed, 47 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/net/ethernet/freescale/fman/mac.c >> b/drivers/net/ethernet/freescale/fman/mac.c >> index 9b863db0bf08..11da139082e1 100644 >> --- a/drivers/net/ethernet/freescale/fman/mac.c >> +++ b/drivers/net/ethernet/freescale/fman/mac.c >> @@ -204,7 +204,7 @@ static int mac_probe(struct platform_device >> *_of_dev) >> if (err) { >> dev_err(dev, "failed to read cell-index for %pOF\n", >> dev_node); >> err = -EINVAL; >> - goto _return_of_node_put; >> + goto _return_dev_put; > > We are after a succesful of_find_device_by_node and prior to > fman_bind(), mac_dev->fman_dev refcount is 1 Indeed. refcounts = 1. > >> @@ -213,40 +213,51 @@ static int mac_probe(struct platform_device >> *_of_dev) >> if (!priv->fman) { >> dev_err(dev, "fman_bind(%pOF) failed\n", dev_node); >> err = -ENODEV; >> - goto _return_of_node_put; >> + goto _return_dev_put; >> } refcounts: 1 + 1 = 2. >> + /* Two references have been taken in of_find_device_by_node() >> + * and fman_bind(). Release one of them here. The second one >> + * will be released in mac_remove(). >> + */ >> + put_device(mac_dev->fman_dev); refcounts: 2 - 1 = 1. >> of_node_put(dev_node); >> + dev_node = NULL; >> /* Get the address of the memory mapped registers */ >> mac_dev->res = platform_get_mem_or_io(_of_dev, 0); >> if (!mac_dev->res) { >> dev_err(dev, "could not get registers\n"); >> - return -EINVAL; >> + err = -EINVAL; >> + goto _return_dev_put; > > Here we are after a successful fman_bind(), mac_dev->fman_dev refcount > is 2. _return_dev_put will drop a single reference, this error path > looks buggy. We released 1 reference above with "put_device(mac_dev->fman_dev);". > > Similar issue for the _return_dev_arr_put error path below. Similar situation: we release 1 reference with "put_device(mac_dev->fman_port_devs[i]);". > > Cheers, > > Paolo >
diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c index 9b863db0bf08..11da139082e1 100644 --- a/drivers/net/ethernet/freescale/fman/mac.c +++ b/drivers/net/ethernet/freescale/fman/mac.c @@ -204,7 +204,7 @@ static int mac_probe(struct platform_device *_of_dev) if (err) { dev_err(dev, "failed to read cell-index for %pOF\n", dev_node); err = -EINVAL; - goto _return_of_node_put; + goto _return_dev_put; } /* cell-index 0 => FMan id 1 */ fman_id = (u8)(val + 1); @@ -213,40 +213,51 @@ static int mac_probe(struct platform_device *_of_dev) if (!priv->fman) { dev_err(dev, "fman_bind(%pOF) failed\n", dev_node); err = -ENODEV; - goto _return_of_node_put; + goto _return_dev_put; } + /* Two references have been taken in of_find_device_by_node() + * and fman_bind(). Release one of them here. The second one + * will be released in mac_remove(). + */ + put_device(mac_dev->fman_dev); of_node_put(dev_node); + dev_node = NULL; /* Get the address of the memory mapped registers */ mac_dev->res = platform_get_mem_or_io(_of_dev, 0); if (!mac_dev->res) { dev_err(dev, "could not get registers\n"); - return -EINVAL; + err = -EINVAL; + goto _return_dev_put; } err = devm_request_resource(dev, fman_get_mem_region(priv->fman), mac_dev->res); if (err) { dev_err_probe(dev, err, "could not request resource\n"); - return err; + goto _return_dev_put; } mac_dev->vaddr = devm_ioremap(dev, mac_dev->res->start, resource_size(mac_dev->res)); if (!mac_dev->vaddr) { dev_err(dev, "devm_ioremap() failed\n"); - return -EIO; + err = -EIO; + goto _return_dev_put; } - if (!of_device_is_available(mac_node)) - return -ENODEV; + if (!of_device_is_available(mac_node)) { + err = -ENODEV; + goto _return_dev_put; + } /* Get the cell-index */ err = of_property_read_u32(mac_node, "cell-index", &val); if (err) { dev_err(dev, "failed to read cell-index for %pOF\n", mac_node); - return -EINVAL; + err = -EINVAL; + goto _return_dev_put; } priv->cell_index = (u8)val; @@ -260,22 +271,26 @@ static int mac_probe(struct platform_device *_of_dev) if (unlikely(nph < 0)) { dev_err(dev, "of_count_phandle_with_args(%pOF, fsl,fman-ports) failed\n", mac_node); - return nph; + err = nph; + goto _return_dev_put; } if (nph != ARRAY_SIZE(mac_dev->port)) { dev_err(dev, "Not supported number of fman-ports handles of mac node %pOF from device tree\n", mac_node); - return -EINVAL; + err = -EINVAL; + goto _return_dev_put; } - for (i = 0; i < ARRAY_SIZE(mac_dev->port); i++) { + /* PORT_NUM determines the size of the port array */ + for (i = 0; i < PORT_NUM; i++) { /* Find the port node */ dev_node = of_parse_phandle(mac_node, "fsl,fman-ports", i); if (!dev_node) { dev_err(dev, "of_parse_phandle(%pOF, fsl,fman-ports) failed\n", mac_node); - return -EINVAL; + err = -EINVAL; + goto _return_dev_arr_put; } of_dev = of_find_device_by_node(dev_node); @@ -283,7 +298,7 @@ static int mac_probe(struct platform_device *_of_dev) dev_err(dev, "of_find_device_by_node(%pOF) failed\n", dev_node); err = -EINVAL; - goto _return_of_node_put; + goto _return_dev_arr_put; } mac_dev->fman_port_devs[i] = &of_dev->dev; @@ -292,9 +307,15 @@ static int mac_probe(struct platform_device *_of_dev) dev_err(dev, "dev_get_drvdata(%pOF) failed\n", dev_node); err = -EINVAL; - goto _return_of_node_put; + goto _return_dev_arr_put; } + /* Two references have been taken in of_find_device_by_node() + * and fman_port_bind(). Release one of them here. The second + * one will be released in mac_remove(). + */ + put_device(mac_dev->fman_port_devs[i]); of_node_put(dev_node); + dev_node = NULL; } /* Get the PHY connection type */ @@ -314,7 +335,7 @@ static int mac_probe(struct platform_device *_of_dev) err = init(mac_dev, mac_node, ¶ms); if (err < 0) - return err; + goto _return_dev_arr_put; if (!is_zero_ether_addr(mac_dev->addr)) dev_info(dev, "FMan MAC address: %pM\n", mac_dev->addr); @@ -329,6 +350,12 @@ static int mac_probe(struct platform_device *_of_dev) return err; +_return_dev_arr_put: + /* mac_dev is kzalloc'ed */ + for (i = 0; i < PORT_NUM; i++) + put_device(mac_dev->fman_port_devs[i]); +_return_dev_put: + put_device(mac_dev->fman_dev); _return_of_node_put: of_node_put(dev_node); return err; @@ -337,6 +364,11 @@ static int mac_probe(struct platform_device *_of_dev) static void mac_remove(struct platform_device *pdev) { struct mac_device *mac_dev = platform_get_drvdata(pdev); + int i; + + for (i = 0; i < PORT_NUM; i++) + put_device(mac_dev->fman_port_devs[i]); + put_device(mac_dev->fman_dev); platform_device_unregister(mac_dev->priv->eth_dev); }
In mac_probe() there are multiple calls to of_find_device_by_node(), fman_bind() and fman_port_bind() which takes references to of_dev->dev. Not all references taken by these calls are released later on error path in mac_probe() and in mac_remove() which lead to reference leaks. Add references release. Fixes: 3933961682a3 ("fsl/fman: Add FMan MAC driver") Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- Compile tested only. drivers/net/ethernet/freescale/fman/mac.c | 62 +++++++++++++++++------ 1 file changed, 47 insertions(+), 15 deletions(-)