Re: [PATCH] i2c: Fix reference leak in of_i2c_register_devices

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Hi,


first of all, you should not reply with HTML mail, because most mailing 
lists reject such messages.


Le 08/04/2025 à 16:01, Sunny a écrit :
> To clarify, while there is no early exit path in the 
> for_each_available_child_of_node() loop, the reference leak can still 
> occur because the macro increments the reference count for each node 
> it processes. If i2c_new_client_device() fails, the reference count 
> for that node must be explicitly decremented using of_node_put(node). 
> Without this, the reference count remains elevated, leading to a 
> reference leak.

I think you are wrong.

Yes, for_each_available_child_of_node() increments the reference count 
for each node it processes, but it also decrements it at the end of the 
iteration, except when there is an early exit (a break or a return).
See how the 'parent' and 'child' parameters of 
of_get_next_available_child() are used, especially whtat happen with the 
first call with child = NULL.

So should i2c_new_client_device() fail or succeed, 'node' is released 
and the reference count does NOT remain elevated as you state.


If you do not agree, please give more details of how you think it works 
and where the issue is, for exemple with unrolling the loop and noting 
what and when nodes are get and put.

You should then see that it is correct.


The of_node_put(node) you are looking for is there: 
https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/of/base.c#L702

CJ

>
> sunny
>
> On Sun, 6 Apr 2025 at 23:36, Christophe JAILLET 
> <christophe.jaillet@wanadoo.fr> wrote:
>
>     Le 06/04/2025 à 15:48, Sunny Patel a écrit :
>     > Fix a potential reference leak in of_i2c_register_devices where the
>     > reference to the node is not released if device registration fails.
>     > This ensures proper reference management and avoids memory leaks.
>
>     There is no early exit path in the for_each_available_child_of_node()
>     block, so of_node_put((node) is called for all the nodes that are
>     iterated.
>
>     Can you elaborate and explain how the reference leak can occur?
>
>     CJ
>
>     >
>     > Signed-off-by: Sunny Patel <nueralspacetech@gmail.com>
>     > ---
>     >   drivers/i2c/i2c-core-of.c | 1 +
>     >   1 file changed, 1 insertion(+)
>     >
>     > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
>     > index 02feee6c9ba9..7c50905de8f1 100644
>     > --- a/drivers/i2c/i2c-core-of.c
>     > +++ b/drivers/i2c/i2c-core-of.c
>     > @@ -107,6 +107,7 @@ void of_i2c_register_devices(struct
>     i2c_adapter *adap)
>     >                                "Failed to create I2C device for
>     %pOF\n",
>     >                                node);
>     >                       of_node_clear_flag(node, OF_POPULATED);
>     > +                     of_node_put(node);
>     >               }
>     >       }
>     >
>