[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20d12a96-c06b-4204-9a57-69a4bac02867@stanley.mountain>
Date: Mon, 14 Oct 2024 11:39:44 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>,
Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Stefan Wahren <wahrenst@....net>,
Umang Jain <umang.jain@...asonboard.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
linux-rpi-kernel@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] staging: vchiq_arm: Fix missing refcount decrement in
error path for fw_node
On Mon, Oct 14, 2024 at 10:15:25AM +0200, Javier Carrasco wrote:
> On 14/10/2024 10:12, Dan Carpenter wrote:
> > On Mon, Oct 14, 2024 at 09:59:49AM +0200, Javier Carrasco wrote:
> >> This approach is great as long as the maintainer accepts mid-scope
> >> variable declaration and the goto instructions get refactored, as stated
> >> in cleanup.h.
> >>
> >> The first point is not being that problematic so far, but the second one
> >> is trickier, and we all have to take special care to avoid such issues,
> >> even if they don't look dangerous in the current code, because adding a
> >> goto where there cleanup attribute is already used can be overlooked as
> >> well.
> >>
> >
> > To be honest, I don't really understand this paragraph. I think maybe you're
> > talking about if we declare the variable at the top and forget to initialize it
> > to NULL? It leads to an uninitialized variable if we exit the function before
> > it is initialized.
> >
>
> No, I am talking about declaring the variable mid-scope, and later on
> adding a goto before that declaration in a different patch, let's say
> far above the variable declaration. As soon as a goto is added, care
> must be taken to make sure that we don't have variables with the cleanup
> attribute in the scope. Just something to take into account.
>
Huh. That's an interesting point. If you have:
if (ret)
goto done;
struct device_node *fw_node __free(device_node) = something;
Then fw_node isn't initialized when we get to done. However, in my simple test
this triggered a build failure with Clang so I believe we would catch this sort
of bug pretty quickly.
regards,
dan carpenter
Powered by blists - more mailing lists