From: Dan Carpenter <dan.carpenter@oracle.com> To: Vladimir Oltean <vladimir.oltean@nxp.com> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, UNGLinuxDriver@microchip.com, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Florian Fainelli <f.fainelli@gmail.com>, netdev@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: [PATCH net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Date: Tue, 2 Feb 2021 12:12:38 +0300 [thread overview] Message-ID: <YBkXhqRxHtRGzSnJ@mwanda> (raw) There are several error handling bugs in mscc_ocelot_init_ports(). I went through the code, and carefully audited it and made fixes and cleanups. 1) The ocelot_probe_port() function didn't have a mirror release function so it was hard to follow. I created the ocelot_release_port() function. 2) In the ocelot_probe_port() function, if the register_netdev() call failed, then it lead to a double free_netdev(dev) bug. Fix this by setting "ocelot->ports[port] = NULL" on the error path. 3) I was concerned that the "port" which comes from of_property_read_u32() might be out of bounds so I added a check for that. 4) In the original code if ocelot_regmap_init() failed then the driver tried to continue but I think that should be a fatal error. 5) If ocelot_probe_port() failed then the most recent devlink was leaked. The fix for mostly came Vladimir Oltean. Get rid of "registered_ports" and just set a bit in "devlink_ports_registered" to say when the devlink port has been registered (and needs to be unregistered on error). There are fewer than 32 ports so a u32 is large enough for this purpose. 6) The error handling if the final ocelot_port_devlink_init() failed had two problems. The "while (port-- >= 0)" loop should have been "--port" pre-op instead of a post-op to avoid a buffer underflow. The "if (!registered_ports[port])" condition was reversed leading to resource leaks and double frees. Fixes: 6c30384eb1de ("net: mscc: ocelot: register devlink ports") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v2: Part of the commit was missing v3: The first version introduced a bug which completely broke the driver. I moved the "ocelot->ports[port] = ocelot_port;" assignment in ocelot_probe_port() and that was wrong. Thanks to Vladimir for catching this. Also v3 has additional cleanups in mscc_ocelot_init_ports(). drivers/net/ethernet/mscc/ocelot.h | 1 + drivers/net/ethernet/mscc/ocelot_net.c | 14 +++++- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 52 ++++++++-------------- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h index e8621dbc14f7..76b8d8ce3b48 100644 --- a/drivers/net/ethernet/mscc/ocelot.h +++ b/drivers/net/ethernet/mscc/ocelot.h @@ -121,6 +121,7 @@ void ocelot_port_writel(struct ocelot_port *port, u32 val, u32 reg); int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, struct phy_device *phy); +void ocelot_release_port(struct ocelot_port *ocelot_port); int ocelot_devlink_init(struct ocelot *ocelot); void ocelot_devlink_teardown(struct ocelot *ocelot); int ocelot_port_devlink_init(struct ocelot *ocelot, int port, diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c index 05142803a463..e6b33d9df184 100644 --- a/drivers/net/ethernet/mscc/ocelot_net.c +++ b/drivers/net/ethernet/mscc/ocelot_net.c @@ -1283,7 +1283,19 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, if (err) { dev_err(ocelot->dev, "register_netdev failed\n"); free_netdev(dev); + ocelot->ports[port] = NULL; + return err; } - return err; + return 0; +} + +void ocelot_release_port(struct ocelot_port *ocelot_port) +{ + struct ocelot_port_private *priv = container_of(ocelot_port, + struct ocelot_port_private, + port); + + unregister_netdev(priv->dev); + free_netdev(priv->dev); } diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index 407244fe5b17..b52e24826b10 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -1064,7 +1064,6 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot) int port; for (port = 0; port < ocelot->num_phys_ports; port++) { - struct ocelot_port_private *priv; struct ocelot_port *ocelot_port; ocelot_port = ocelot->ports[port]; @@ -1072,12 +1071,7 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot) continue; ocelot_deinit_port(ocelot, port); - - priv = container_of(ocelot_port, struct ocelot_port_private, - port); - - unregister_netdev(priv->dev); - free_netdev(priv->dev); + ocelot_release_port(ocelot_port); } } @@ -1085,8 +1079,8 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, struct device_node *ports) { struct ocelot *ocelot = platform_get_drvdata(pdev); + u32 devlink_ports_registered = 0; struct device_node *portnp; - bool *registered_ports; int port, err; u32 reg; @@ -1102,11 +1096,6 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, if (!ocelot->devlink_ports) return -ENOMEM; - registered_ports = kcalloc(ocelot->num_phys_ports, sizeof(bool), - GFP_KERNEL); - if (!registered_ports) - return -ENOMEM; - for_each_available_child_of_node(ports, portnp) { struct ocelot_port_private *priv; struct ocelot_port *ocelot_port; @@ -1123,14 +1112,22 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, continue; port = reg; + if (port < 0 || port >= ocelot->num_phys_ports) { + dev_err(ocelot->dev, + "invalid port number: %d >= %d\n", port, + ocelot->num_phys_ports); + continue; + } snprintf(res_name, sizeof(res_name), "port%d", port); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, res_name); target = ocelot_regmap_init(ocelot, res); - if (IS_ERR(target)) - continue; + if (IS_ERR(target)) { + err = PTR_ERR(target); + goto out_teardown; + } phy_node = of_parse_phandle(portnp, "phy-handle", 0); if (!phy_node) @@ -1147,6 +1144,7 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, of_node_put(portnp); goto out_teardown; } + devlink_ports_registered |= BIT(port); err = ocelot_probe_port(ocelot, port, target, phy); if (err) { @@ -1154,8 +1152,6 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, goto out_teardown; } - registered_ports[port] = true; - ocelot_port = ocelot->ports[port]; priv = container_of(ocelot_port, struct ocelot_port_private, port); @@ -1208,23 +1204,16 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, /* Initialize unused devlink ports at the end */ for (port = 0; port < ocelot->num_phys_ports; port++) { - if (registered_ports[port]) + if (devlink_ports_registered & BIT(port)) continue; err = ocelot_port_devlink_init(ocelot, port, DEVLINK_PORT_FLAVOUR_UNUSED); - if (err) { - while (port-- >= 0) { - if (!registered_ports[port]) - continue; - ocelot_port_devlink_teardown(ocelot, port); - } - + if (err) goto out_teardown; - } - } - kfree(registered_ports); + devlink_ports_registered |= BIT(port); + } return 0; @@ -1233,12 +1222,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, mscc_ocelot_release_ports(ocelot); /* Tear down devlink ports for the registered network interfaces */ for (port = 0; port < ocelot->num_phys_ports; port++) { - if (!registered_ports[port]) - continue; - - ocelot_port_devlink_teardown(ocelot, port); + if (devlink_ports_registered & BIT(port)) + ocelot_port_devlink_teardown(ocelot, port); } - kfree(registered_ports); return err; } -- 2.30.0
WARNING: multiple messages have this Message-ID (diff)
From: Dan Carpenter <dan.carpenter@oracle.com> To: Vladimir Oltean <vladimir.oltean@nxp.com> Cc: Claudiu Manoil <claudiu.manoil@nxp.com>, Alexandre Belloni <alexandre.belloni@bootlin.com>, UNGLinuxDriver@microchip.com, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Florian Fainelli <f.fainelli@gmail.com>, netdev@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: [PATCH net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Date: Tue, 02 Feb 2021 09:12:38 +0000 [thread overview] Message-ID: <YBkXhqRxHtRGzSnJ@mwanda> (raw) There are several error handling bugs in mscc_ocelot_init_ports(). I went through the code, and carefully audited it and made fixes and cleanups. 1) The ocelot_probe_port() function didn't have a mirror release function so it was hard to follow. I created the ocelot_release_port() function. 2) In the ocelot_probe_port() function, if the register_netdev() call failed, then it lead to a double free_netdev(dev) bug. Fix this by setting "ocelot->ports[port] = NULL" on the error path. 3) I was concerned that the "port" which comes from of_property_read_u32() might be out of bounds so I added a check for that. 4) In the original code if ocelot_regmap_init() failed then the driver tried to continue but I think that should be a fatal error. 5) If ocelot_probe_port() failed then the most recent devlink was leaked. The fix for mostly came Vladimir Oltean. Get rid of "registered_ports" and just set a bit in "devlink_ports_registered" to say when the devlink port has been registered (and needs to be unregistered on error). There are fewer than 32 ports so a u32 is large enough for this purpose. 6) The error handling if the final ocelot_port_devlink_init() failed had two problems. The "while (port-- >= 0)" loop should have been "--port" pre-op instead of a post-op to avoid a buffer underflow. The "if (!registered_ports[port])" condition was reversed leading to resource leaks and double frees. Fixes: 6c30384eb1de ("net: mscc: ocelot: register devlink ports") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v2: Part of the commit was missing v3: The first version introduced a bug which completely broke the driver. I moved the "ocelot->ports[port] = ocelot_port;" assignment in ocelot_probe_port() and that was wrong. Thanks to Vladimir for catching this. Also v3 has additional cleanups in mscc_ocelot_init_ports(). drivers/net/ethernet/mscc/ocelot.h | 1 + drivers/net/ethernet/mscc/ocelot_net.c | 14 +++++- drivers/net/ethernet/mscc/ocelot_vsc7514.c | 52 ++++++++-------------- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h index e8621dbc14f7..76b8d8ce3b48 100644 --- a/drivers/net/ethernet/mscc/ocelot.h +++ b/drivers/net/ethernet/mscc/ocelot.h @@ -121,6 +121,7 @@ void ocelot_port_writel(struct ocelot_port *port, u32 val, u32 reg); int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, struct phy_device *phy); +void ocelot_release_port(struct ocelot_port *ocelot_port); int ocelot_devlink_init(struct ocelot *ocelot); void ocelot_devlink_teardown(struct ocelot *ocelot); int ocelot_port_devlink_init(struct ocelot *ocelot, int port, diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c index 05142803a463..e6b33d9df184 100644 --- a/drivers/net/ethernet/mscc/ocelot_net.c +++ b/drivers/net/ethernet/mscc/ocelot_net.c @@ -1283,7 +1283,19 @@ int ocelot_probe_port(struct ocelot *ocelot, int port, struct regmap *target, if (err) { dev_err(ocelot->dev, "register_netdev failed\n"); free_netdev(dev); + ocelot->ports[port] = NULL; + return err; } - return err; + return 0; +} + +void ocelot_release_port(struct ocelot_port *ocelot_port) +{ + struct ocelot_port_private *priv = container_of(ocelot_port, + struct ocelot_port_private, + port); + + unregister_netdev(priv->dev); + free_netdev(priv->dev); } diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index 407244fe5b17..b52e24826b10 100644 --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -1064,7 +1064,6 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot) int port; for (port = 0; port < ocelot->num_phys_ports; port++) { - struct ocelot_port_private *priv; struct ocelot_port *ocelot_port; ocelot_port = ocelot->ports[port]; @@ -1072,12 +1071,7 @@ static void mscc_ocelot_release_ports(struct ocelot *ocelot) continue; ocelot_deinit_port(ocelot, port); - - priv = container_of(ocelot_port, struct ocelot_port_private, - port); - - unregister_netdev(priv->dev); - free_netdev(priv->dev); + ocelot_release_port(ocelot_port); } } @@ -1085,8 +1079,8 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, struct device_node *ports) { struct ocelot *ocelot = platform_get_drvdata(pdev); + u32 devlink_ports_registered = 0; struct device_node *portnp; - bool *registered_ports; int port, err; u32 reg; @@ -1102,11 +1096,6 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, if (!ocelot->devlink_ports) return -ENOMEM; - registered_ports = kcalloc(ocelot->num_phys_ports, sizeof(bool), - GFP_KERNEL); - if (!registered_ports) - return -ENOMEM; - for_each_available_child_of_node(ports, portnp) { struct ocelot_port_private *priv; struct ocelot_port *ocelot_port; @@ -1123,14 +1112,22 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, continue; port = reg; + if (port < 0 || port >= ocelot->num_phys_ports) { + dev_err(ocelot->dev, + "invalid port number: %d >= %d\n", port, + ocelot->num_phys_ports); + continue; + } snprintf(res_name, sizeof(res_name), "port%d", port); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, res_name); target = ocelot_regmap_init(ocelot, res); - if (IS_ERR(target)) - continue; + if (IS_ERR(target)) { + err = PTR_ERR(target); + goto out_teardown; + } phy_node = of_parse_phandle(portnp, "phy-handle", 0); if (!phy_node) @@ -1147,6 +1144,7 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, of_node_put(portnp); goto out_teardown; } + devlink_ports_registered |= BIT(port); err = ocelot_probe_port(ocelot, port, target, phy); if (err) { @@ -1154,8 +1152,6 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, goto out_teardown; } - registered_ports[port] = true; - ocelot_port = ocelot->ports[port]; priv = container_of(ocelot_port, struct ocelot_port_private, port); @@ -1208,23 +1204,16 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, /* Initialize unused devlink ports at the end */ for (port = 0; port < ocelot->num_phys_ports; port++) { - if (registered_ports[port]) + if (devlink_ports_registered & BIT(port)) continue; err = ocelot_port_devlink_init(ocelot, port, DEVLINK_PORT_FLAVOUR_UNUSED); - if (err) { - while (port-- >= 0) { - if (!registered_ports[port]) - continue; - ocelot_port_devlink_teardown(ocelot, port); - } - + if (err) goto out_teardown; - } - } - kfree(registered_ports); + devlink_ports_registered |= BIT(port); + } return 0; @@ -1233,12 +1222,9 @@ static int mscc_ocelot_init_ports(struct platform_device *pdev, mscc_ocelot_release_ports(ocelot); /* Tear down devlink ports for the registered network interfaces */ for (port = 0; port < ocelot->num_phys_ports; port++) { - if (!registered_ports[port]) - continue; - - ocelot_port_devlink_teardown(ocelot, port); + if (devlink_ports_registered & BIT(port)) + ocelot_port_devlink_teardown(ocelot, port); } - kfree(registered_ports); return err; } -- 2.30.0
next reply other threads:[~2021-02-02 9:16 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-02 9:12 Dan Carpenter [this message] 2021-02-02 9:12 ` [PATCH net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() Dan Carpenter 2021-02-02 9:13 ` [PATCH v3 2/2 net-next] net: mscc: ocelot: fix error code in mscc_ocelot_probe() Dan Carpenter 2021-02-02 9:13 ` Dan Carpenter 2021-02-04 1:30 ` [PATCH net-next] net: mscc: ocelot: fix error handling bugs in mscc_ocelot_init_ports() patchwork-bot+netdevbpf
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YBkXhqRxHtRGzSnJ@mwanda \ --to=dan.carpenter@oracle.com \ --cc=UNGLinuxDriver@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=claudiu.manoil@nxp.com \ --cc=davem@davemloft.net \ --cc=f.fainelli@gmail.com \ --cc=kernel-janitors@vger.kernel.org \ --cc=kuba@kernel.org \ --cc=netdev@vger.kernel.org \ --cc=vladimir.oltean@nxp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.