[PATCH 0/3] drm: logicvc: add of_node_put() and switch to a more secure approach

[PATCH 0/3] drm: logicvc: add of_node_put() and switch to a more secure approach

[Javier Carrasco]

This driver has faced several issues due to the wrong or missing usage
of of_node_put() to release device nodes after they are no longer
required.

The first implementation was missing the of_node_put() for
'layers_node', and it put 'layer_node' twice. Then commit
'd3a453416270 ("drm: fix device_node_continue.cocci warnings")'
removed the extra of_node_put(layer_node), which would have been ok if
it had stayed only in the error path. Later, commit
'e9fcc60ddd29 ("drm/logicvc: add missing of_node_put() in
logicvc_layers_init()")' added the missing of_node_put(layers_node),
but not the one for the child node.

It should be clear how easy someone can mess up with this pattern,
especially with variables that have similar names.

To fix the bug for stable kernels, and provide a more robust solution
that accounts for new error paths, this series provides a first patch
with the classical approach of adding the missing of_node_put(), and two
more patches to use the cleanup attribute and avoid issues with
device nodes again.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (3):
      drm: logicvc: fix missing of_node_put() in for_each_child_of_node()
      drm: logicvc: switch to for_each_child_of_node_scoped()
      drm: logicvc: use automatic cleanup facility for layers_node

 drivers/gpu/drm/logicvc/logicvc_layer.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)
---
base-commit: 0cca97bf23640ff68a6e8a74e9b6659fdc27f48c
change-id: 20241010-logicvc_layer_of_node_put-bc4cb207280b

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/3] drm: logicvc: fix missing of_node_put() in for_each_child_of_node()

[Javier Carrasco]

Early exits from the for_each_child_of_node() loop require explicit
calls to of_node_put() for the child node.

Add the missing 'of_node_put(layer_node)' in the only error path.

Cc: stable@vger.kernel.org
Fixes: efeeaefe9be5 ("drm: Add support for the LogiCVC display controller")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/gpu/drm/logicvc/logicvc_layer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
index 464000aea765..52dabacd42ee 100644
--- a/drivers/gpu/drm/logicvc/logicvc_layer.c
+++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
@@ -613,6 +613,7 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
 
 		ret = logicvc_layer_init(logicvc, layer_node, index);
 		if (ret) {
+			of_node_put(layer_node);
 			of_node_put(layers_node);
 			goto error;
 		}

-- 
2.43.0

Re: [PATCH 1/3] drm: logicvc: fix missing of_node_put() in for_each_child_of_node()

[Louis Chauvet]

On 11/10/24 - 01:11, Javier Carrasco wrote:
> Early exits from the for_each_child_of_node() loop require explicit
> calls to of_node_put() for the child node.
> 
> Add the missing 'of_node_put(layer_node)' in the only error path.
> 
> Cc: stable@vger.kernel.org
> Fixes: efeeaefe9be5 ("drm: Add support for the LogiCVC display controller")
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Reviewed-By: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
>  drivers/gpu/drm/logicvc/logicvc_layer.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
> index 464000aea765..52dabacd42ee 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_layer.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
> @@ -613,6 +613,7 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
>  
>  		ret = logicvc_layer_init(logicvc, layer_node, index);
>  		if (ret) {
> +			of_node_put(layer_node);
>  			of_node_put(layers_node);
>  			goto error;
>  		}
> 
> -- 
> 2.43.0
> 

[PATCH 2/3] drm: logicvc: switch to for_each_child_of_node_scoped()

[Javier Carrasco]

Use the scoped variant of the macro to avoid leaking memory upon early
exits without the required call to of_node_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/gpu/drm/logicvc/logicvc_layer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
index 52dabacd42ee..34caf5f0f619 100644
--- a/drivers/gpu/drm/logicvc/logicvc_layer.c
+++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
@@ -581,7 +581,6 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
 	struct drm_device *drm_dev = &logicvc->drm_dev;
 	struct device *dev = drm_dev->dev;
 	struct device_node *of_node = dev->of_node;
-	struct device_node *layer_node = NULL;
 	struct device_node *layers_node;
 	struct logicvc_layer *layer;
 	struct logicvc_layer *next;
@@ -594,7 +593,7 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
 		goto error;
 	}
 
-	for_each_child_of_node(layers_node, layer_node) {
+	for_each_child_of_node_scoped(layers_node, layer_node) {
 		u32 index = 0;
 
 		if (!logicvc_of_node_is_layer(layer_node))
@@ -613,7 +612,6 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
 
 		ret = logicvc_layer_init(logicvc, layer_node, index);
 		if (ret) {
-			of_node_put(layer_node);
 			of_node_put(layers_node);
 			goto error;
 		}

-- 
2.43.0

Re: [PATCH 2/3] drm: logicvc: switch to for_each_child_of_node_scoped()

[Louis Chauvet]

Hi,

I think you can squash this commit with the prvious one, I don't think 
this is needed to add of_node_put and remove it just after.

Thanks,
Louis Chauvet

On 11/10/24 - 01:11, Javier Carrasco wrote:
> Use the scoped variant of the macro to avoid leaking memory upon early
> exits without the required call to of_node_put().
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/gpu/drm/logicvc/logicvc_layer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
> index 52dabacd42ee..34caf5f0f619 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_layer.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
> @@ -581,7 +581,6 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
>  	struct drm_device *drm_dev = &logicvc->drm_dev;
>  	struct device *dev = drm_dev->dev;
>  	struct device_node *of_node = dev->of_node;
> -	struct device_node *layer_node = NULL;
>  	struct device_node *layers_node;
>  	struct logicvc_layer *layer;
>  	struct logicvc_layer *next;
> @@ -594,7 +593,7 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
>  		goto error;
>  	}
>  
> -	for_each_child_of_node(layers_node, layer_node) {
> +	for_each_child_of_node_scoped(layers_node, layer_node) {
>  		u32 index = 0;
>  
>  		if (!logicvc_of_node_is_layer(layer_node))
> @@ -613,7 +612,6 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
>  
>  		ret = logicvc_layer_init(logicvc, layer_node, index);
>  		if (ret) {
> -			of_node_put(layer_node);
>  			of_node_put(layers_node);
>  			goto error;
>  		}
> 
> -- 
> 2.43.0
> 

Re: [PATCH 2/3] drm: logicvc: switch to for_each_child_of_node_scoped()

[Louis Chauvet]

On 11/10/24 - 15:01, Louis Chauvet wrote:
> Hi,
> 
> I think you can squash this commit with the prvious one, I don't think 
> this is needed to add of_node_put and remove it just after.

Forget this, I missed the Fixes in the first commit, sorry for the noise.

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> Thanks,
> Louis Chauvet
> 
> On 11/10/24 - 01:11, Javier Carrasco wrote:
> > Use the scoped variant of the macro to avoid leaking memory upon early
> > exits without the required call to of_node_put().
> > 
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---
> >  drivers/gpu/drm/logicvc/logicvc_layer.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
> > index 52dabacd42ee..34caf5f0f619 100644
> > --- a/drivers/gpu/drm/logicvc/logicvc_layer.c
> > +++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
> > @@ -581,7 +581,6 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
> >  	struct drm_device *drm_dev = &logicvc->drm_dev;
> >  	struct device *dev = drm_dev->dev;
> >  	struct device_node *of_node = dev->of_node;
> > -	struct device_node *layer_node = NULL;
> >  	struct device_node *layers_node;
> >  	struct logicvc_layer *layer;
> >  	struct logicvc_layer *next;
> > @@ -594,7 +593,7 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
> >  		goto error;
> >  	}
> >  
> > -	for_each_child_of_node(layers_node, layer_node) {
> > +	for_each_child_of_node_scoped(layers_node, layer_node) {
> >  		u32 index = 0;
> >  
> >  		if (!logicvc_of_node_is_layer(layer_node))
> > @@ -613,7 +612,6 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
> >  
> >  		ret = logicvc_layer_init(logicvc, layer_node, index);
> >  		if (ret) {
> > -			of_node_put(layer_node);
> >  			of_node_put(layers_node);
> >  			goto error;
> >  		}
> > 
> > -- 
> > 2.43.0
> > 

Re: [PATCH 2/3] drm: logicvc: switch to for_each_child_of_node_scoped()

[Javier Carrasco]

On 11/10/2024 15:01, Louis Chauvet wrote:
> Hi,
> 
> I think you can squash this commit with the prvious one, I don't think 
> this is needed to add of_node_put and remove it just after.
> 
> Thanks,
> Louis Chauvet
> 

Hi Louis,

Thanks for your review. I did not squash them because the first one
would apply to stable patches (note the Cc: tag) that are older than the
scoped macro, which was introduced this year.

Best regards,
Javier Carrasco

[PATCH 3/3] drm: logicvc: use automatic cleanup facility for layers_node

[Javier Carrasco]

Use the more robust approach provided by the __free() macro to
automatically call of_node_put() when the device node goes out of scope.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/gpu/drm/logicvc/logicvc_layer.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
index 34caf5f0f619..9d7d1b58b002 100644
--- a/drivers/gpu/drm/logicvc/logicvc_layer.c
+++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
@@ -581,12 +581,12 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
 	struct drm_device *drm_dev = &logicvc->drm_dev;
 	struct device *dev = drm_dev->dev;
 	struct device_node *of_node = dev->of_node;
-	struct device_node *layers_node;
+	struct device_node *layers_node __free(device_node) =
+		of_get_child_by_name(of_node, "layers");
 	struct logicvc_layer *layer;
 	struct logicvc_layer *next;
 	int ret = 0;
 
-	layers_node = of_get_child_by_name(of_node, "layers");
 	if (!layers_node) {
 		drm_err(drm_dev, "No layers node found in the description\n");
 		ret = -ENODEV;
@@ -611,14 +611,10 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
 		}
 
 		ret = logicvc_layer_init(logicvc, layer_node, index);
-		if (ret) {
-			of_node_put(layers_node);
+		if (ret)
 			goto error;
-		}
 	}
 
-	of_node_put(layers_node);
-
 	return 0;
 
 error:

-- 
2.43.0

Re: [PATCH 3/3] drm: logicvc: use automatic cleanup facility for layers_node

[Louis Chauvet]

On 11/10/24 - 01:11, Javier Carrasco wrote:
> Use the more robust approach provided by the __free() macro to
> automatically call of_node_put() when the device node goes out of scope.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> ---
>  drivers/gpu/drm/logicvc/logicvc_layer.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
> index 34caf5f0f619..9d7d1b58b002 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_layer.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
> @@ -581,12 +581,12 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
>  	struct drm_device *drm_dev = &logicvc->drm_dev;
>  	struct device *dev = drm_dev->dev;
>  	struct device_node *of_node = dev->of_node;
> -	struct device_node *layers_node;
> +	struct device_node *layers_node __free(device_node) =
> +		of_get_child_by_name(of_node, "layers");
>  	struct logicvc_layer *layer;
>  	struct logicvc_layer *next;
>  	int ret = 0;
>  
> -	layers_node = of_get_child_by_name(of_node, "layers");
>  	if (!layers_node) {
>  		drm_err(drm_dev, "No layers node found in the description\n");
>  		ret = -ENODEV;
> @@ -611,14 +611,10 @@ int logicvc_layers_init(struct logicvc_drm *logicvc)
>  		}
>  
>  		ret = logicvc_layer_init(logicvc, layer_node, index);
> -		if (ret) {
> -			of_node_put(layers_node);
> +		if (ret)
>  			goto error;
> -		}
>  	}
>  
> -	of_node_put(layers_node);
> -
>  	return 0;
>  
>  error:
> 
> -- 
> 2.43.0
>