linux-kernel - Re: [PATCH v1 09/18] driver core: Allow only unprobed consumers for SYNC_STATE

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGETcx-DxkuCE2EuiBoevXobVb55M5WXa+d1syK2u-rUu6Z7Jw@mail.gmail.com>
Date:   Fri, 20 Nov 2020 17:59:44 -0800
From:   Saravana Kannan <saravanak@...gle.com>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Marc Zyngier <maz@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tomi Valkeinen <tomi.valkeinen@...com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Grygorii Strashko <grygorii.strashko@...com>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-efi <linux-efi@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v1 09/18] driver core: Allow only unprobed consumers for
 SYNC_STATE_ONLY device links

On Mon, Nov 16, 2020 at 7:57 AM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Thu, Nov 5, 2020 at 12:24 AM Saravana Kannan <saravanak@...gle.com> wrote:
> >
> > SYNC_STATE_ONLY device links only affect the behavior of sync_state()
> > callbacks. Specifically, they prevent sync_state() only callbacks from
> > being called on a device if one or more of its consumers haven't probed.
> >
> > So, creating a SYNC_STATE_ONLY device link from an already probed
> > consumer is useless. So, don't allow creating such device links.
>
> I'm wondering why this needs to be part of the series?
>
> It looks like it could go in separately, couldn't it?

Right, I just wrote this as part of the series as I noticed this gap
in the error checking as I wrote this series. It can go in separately.

>
> >
> > Signed-off-by: Saravana Kannan <saravanak@...gle.com>
> > ---
> >  drivers/base/core.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 1a1d9a55645c..4a0907574646 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -646,6 +646,17 @@ struct device_link *device_link_add(struct device *consumer,
> >                 goto out;
> >         }
> >
> > +       /*
> > +        * SYNC_STATE_ONLY links are useless once a consumer device has probed.
> > +        * So, only create it if the consumer hasn't probed yet.
> > +        */
> > +       if (flags & DL_FLAG_SYNC_STATE_ONLY &&
> > +           consumer->links.status != DL_DEV_NO_DRIVER &&
> > +           consumer->links.status != DL_DEV_PROBING) {
> > +               link = NULL;
> > +               goto out;
> > +       }
>
> Returning NULL at this point may be confusing if there is a link
> between these devices already.

But the request is for a SYNC_STATE_ONLY link that can't be created
when this condition is met. I see it similar to the error check above.

I think returning the existing non-SYNC_STATE_ONLY link gives the
wrong impression that the link was created successfully. Also, if I
find the existing link and return it, then I need to refcount it
(conditional on STATELESS?) and
the caller who shouldn't be trying to create this link should now need
to keep track of this and release it too. I think it's cleaner and
simpler to just return NULL.


-Saravana