Received-SPF: pass (google.com: domain of linux-kernel+bounces-158033-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 25 Apr 2024 07:50:03 +0200 (CEST)
From: Julia Lawall <julia.lawall@inria.fr>
To: Kees Cook <keescook@chromium.org>
cc: Abhinav Jain <jain.abhinav177@gmail.com>, tony.luck@intel.com, 
    gpiccoli@igalia.com, linux-hardening@vger.kernel.org, 
    linux-kernel@vger.kernel.org, skhan@linuxfoundation.org, 
    javier.carrasco.cruz@gmail.com
Subject: Re: [PATCH] pstore/ram: Replace of_node_put with __free() for
 automatic cleanup
In-Reply-To: <202404241639.3F455ECCD@keescook>
Message-ID: <alpine.DEB.2.22.394.2404250745590.3674@hadrien>
References: <20240415161409.8375-1-jain.abhinav177@gmail.com> <202404241639.3F455ECCD@keescook>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII


On Wed, 24 Apr 2024, Kees Cook wrote:

> On Mon, Apr 15, 2024 at 04:14:09PM +0000, Abhinav Jain wrote:
> > Add __free(device_node) to the parent_node struct declaration.
> > Move declaration to initialization for ensuring scope sanity.
> > Remove of_node_put from parent_node struct.
> >
> > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com>
> > ---
> >  fs/pstore/ram.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index b1a455f42e93..14f2f4864e48 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -644,7 +644,6 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >  			    struct ramoops_platform_data *pdata)
> >  {
> >  	struct device_node *of_node = pdev->dev.of_node;
> > -	struct device_node *parent_node;
> >  	struct resource *res;
> >  	u32 value;
> >  	int ret;
> > @@ -704,14 +703,13 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >  	 * we're not a child of "reserved-memory" and mimicking the
> >  	 * expected behavior.
> >  	 */
> > -	parent_node = of_get_parent(of_node);
> > +	struct device_node *parent_node __free(device_node) = of_node_parent(of_node);
>
> Please don't move variable definitions into the middle of the function
> body. :)

This is done in other cases where it makes more sense to put the
initialization later in the function.  The point is that the variable has
to be initialized, and puttng the declaration lower, which is now allowed
when needed, is better than first uselessly initializing the variable to
NULL.

>
> >  	if (!of_node_name_eq(parent_node, "reserved-memory") &&
> >  	    !pdata->console_size && !pdata->ftrace_size &&
> >  	    !pdata->pmsg_size && !pdata->ecc_info.ecc_size) {
> >  		pdata->console_size = pdata->record_size;
> >  		pdata->pmsg_size = pdata->record_size;
> >  	}
> > -	of_node_put(parent_node);
>
> So this change is functionally fine, but there's really no good reason
> to do this -- there is no fancy error handling here, so there's no
> benefit to making this change. It doesn't really help readability.

The benefit is general consistency across the code base.  If we could just
get rid of all local-scoped of_node_puts, then we could more easily check
that device nodes are safely used, without having to study the rest of the
code.

Just my opinion.  You decide.

julia