linux-kernel - Re: [PATCH] coccinelle: put_device: Include of_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.22.394.2302262130430.2761@hadrien>
Date:   Sun, 26 Feb 2023 21:33:34 +0100 (CET)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Deepak R Varma <drv@...lo.com>
cc:     nicolas palix <nicolas.palix@...g.fr>, cocci <cocci@...ia.fr>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Saurabh Singh Sengar <ssengar@...rosoft.com>,
        Praveen Kumar <kumarpraveen@...ux.microsoft.com>
Subject: Re: [PATCH] coccinelle: put_device: Include of_node_put to avoid
 false positives



On Mon, 27 Feb 2023, Deepak R Varma wrote:

> On Sat, Feb 25, 2023 at 08:17:01PM +0100, Julia Lawall wrote:
> > > The node reference increased by of_find_device_by_node() can also be
> > > released by using a call to of_node_put(). Hence when this exists, the
> > > script should not trigger a warning, which otherwise will be a false
> > > positive.
> >
> > Could you explain more about why of_node_put is sufficient?
>
> You are right. In fact, I think calling of_node_put() for a prior
> of_find_device_by_node() is buggy as a call to of_find_device_by_node()
> increments the kref for the device embedding the node and not the kref of the
> node. Hence a call to put_device() is required to decrement the device kref.
> Calling the of_node_put() attempts to decrement the kref of the node, which I
> think is not correct.
>
> May I request any comment on my understanding?

I think that decrementing a kref and reaching 0 would trigger some cleanup
action.  I don't know what that cleanup action is in this case.

Did someone tell you that of_node_put was good enough?  Perhaps that
person could provide more explanation.

In looking quickly though the code, the focus seemed to be on the device.
The of node is just used for comparison to check that we have the right
one.  But I don't know if cleaning up the of node could somehow trigger
cleaning up the device as well.

julia

>
> Thank you,
> deepak.
>
> >
> > thanks,
> > julia
> >
> > > Also, improve the warning message to include of_node_put too is missing.
> > >
> > > Signed-off-by: Deepak R Varma <drv@...lo.com>
> > > ---
> > > scripts/coccinelle/free/put_device.cocci | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/coccinelle/free/put_device.cocci
> > > b/scripts/coccinelle/free/put_device.cocci
> > > index f09f1e79bfa6..259195b501aa 100644
> > > --- a/scripts/coccinelle/free/put_device.cocci
> > > +++ b/scripts/coccinelle/free/put_device.cocci
> > > @@ -18,8 +18,10 @@ type T,T1,T2,T3;
> > >
> > > id = of_find_device_by_node@p1(x)
> > > ... when != e = id
> > > +    when != of_node_put(x)
> > > if (id == NULL || ...) { ... return ...; }
> > > ... when != put_device(&id->dev)
> > > +    when != of_node_put(x)
> > >     when != platform_device_put(id)
> > >     when != if (id) { ... put_device(&id->dev) ... }
> > >     when != e1 = (T)id
> > > @@ -42,7 +44,7 @@ p2 << search.p2;
> > > @@
> > >
> > > coccilib.report.print_report(p2[0],
> > > -                             "ERROR: missing put_device; call
> > > of_find_device_by_node on line "
> > > +                             "ERROR: missing put_device or of_node_put; call
> > > of_find_device_by_node on line "
> > >                              + p1[0].line
> > >                              + ", but without a corresponding object release within this function.")
> > >
> > > --
> > > 2.34.1
>
>
>