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
Date: Thu, 25 Apr 2024 07:50:03 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.22.394.2404250745590.3674@hadrien> (raw)
In-Reply-To: <202404241639.3F455ECCD@keescook>
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
next prev parent reply other threads:[~2024-04-25 5:50 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-15 16:14 [PATCH] pstore/ram: Replace of_node_put with __free() for automatic cleanup Abhinav Jain
2024-04-24 23:41 ` Kees Cook
2024-04-25 5:50 ` Julia Lawall [this message]
2024-06-05 22:10 ` Abhinav Jain
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=alpine.DEB.2.22.394.2404250745590.3674@hadrien \
--to=julia.lawall@inria.fr \
--cc=gpiccoli@igalia.com \
--cc=jain.abhinav177@gmail.com \
--cc=javier.carrasco.cruz@gmail.com \
--cc=keescook@chromium.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=tony.luck@intel.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).