[PATCH v3] net: ethernet: renesas: rswitch: fix device_node refcount leak in rswitch_get_port_node()

[PATCH v3] net: ethernet: renesas: rswitch: fix device_node refcount leak in rswitch_get_port_node()

[Manush Prajwal]

On an of_property_read_u32() failure, rswitch_get_port_node() set port
to NULL and jumped to the out label before releasing the reference the
for_each_available_child_of_node() iterator was holding on it. Once
port was overwritten with NULL, that reference could never be
released since out: only put "ports", the parent node.

Rework the function around for_each_available_child_of_node_scoped()
instead of adding a manual of_node_put(), so the iterator's reference
is dropped automatically on every exit path. Since port is the
function's return value, take an explicit reference with of_node_get()
on the match before breaking out of the loop.

Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
---
v3: Reorder local variable declarations into reverse Christmas tree
    order, per Andrew Lunn's review.
v2: Rework using for_each_available_child_of_node_scoped() instead of
    a manual of_node_put(), per Andrew Lunn's review.

 drivers/net/ethernet/renesas/rswitch_main.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
index 6fe9648163..ffdc42f430 100644
--- a/drivers/net/ethernet/renesas/rswitch_main.c
+++ b/drivers/net/ethernet/renesas/rswitch_main.c
@@ -1303,8 +1303,8 @@
 /* Call of_node_put(port) after done */
 static struct device_node *rswitch_get_port_node(struct rswitch_device *rdev)
 {
-	struct device_node *ports, *port;
-	int err = 0;
+	struct device_node *port = NULL;
+	struct device_node *ports;
 	u32 index;

 	ports = of_get_child_by_name(rdev->ndev->dev.parent->of_node,
@@ -1312,17 +1312,15 @@ static struct device_node *rswitch_get_port_node(struct rswitch_device *rdev)
 	if (!ports)
 		return NULL;

-	for_each_available_child_of_node(ports, port) {
-		err = of_property_read_u32(port, "reg", &index);
-		if (err < 0) {
-			port = NULL;
-			goto out;
-		}
-		if (index == rdev->etha->index)
+	for_each_available_child_of_node_scoped(ports, child) {
+		if (of_property_read_u32(child, "reg", &index))
+			break;
+		if (index == rdev->etha->index) {
+			port = of_node_get(child);
 			break;
+		}
 	}

-out:
 	of_node_put(ports);

 	return port;
--
2.46.2.windows.1

Re: [PATCH v3] net: ethernet: renesas: rswitch: fix device_node refcount leak in rswitch_get_port_node()

[Andrew Lunn]

> -	for_each_available_child_of_node(ports, port) {
> -		err = of_property_read_u32(port, "reg", &index);
> -		if (err < 0) {
> -			port = NULL;
> -			goto out;
> -		}
> -		if (index == rdev->etha->index)
> +	for_each_available_child_of_node_scoped(ports, child) {

> +		if (of_property_read_u32(child, "reg", &index))
> +			break;

Why did you change this?

Please keep the changes minimal, only change what must be changed.

I doubt you have tested this code in any way, so the change has
risk. By keeping the change as small as possible, you keep the risk
lower, so you are less likely to break it.

	Andrew