linux-kernel - Re: [PATCH v5 09/13] usb: roles: Add usb role switch notifier.

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: <CALAqxLUQW03waz0LETcnYpc7qhm=RG6RfwbGVZ7MQz00jTZSWQ@mail.gmail.com>
Date:   Thu, 11 Apr 2019 18:12:30 -0700
From:   John Stultz <john.stultz@...aro.org>
To:     Yu Chen <chenyu56@...wei.com>
Cc:     Linux USB List <linux-usb@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, lkml <linux-kernel@...r.kernel.org>,
        Zhuangluan Su <suzhuangluan@...ilicon.com>,
        Kongfei <kongfei@...ilicon.com>,
        "Liuyu (R)" <liuyu712@...ilicon.com>, wanghu17@...ilicon.com,
        butao <butao@...ilicon.com>, chenyao11@...wei.com,
        fangshengzhou@...ilicon.com,
        Li Pengcheng <lipengcheng8@...wei.com>,
        songxiaowei@...ilicon.com, YiPing Xu <xuyiping@...ilicon.com>,
        xuyoujun4@...wei.com, Yudongbin <yudongbin@...ilicon.com>,
        Zang Leigang <zangleigang@...ilicon.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v5 09/13] usb: roles: Add usb role switch notifier.

On Thu, Mar 28, 2019 at 9:14 PM Yu Chen <chenyu56@...wei.com> wrote:
>
> This patch adds notifier for drivers want to be informed of the usb role
> switch.
>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> Cc: Hans de Goede <hdegoede@...hat.com>
> Cc: Andy Shevchenko <andy.shevchenko@...il.com>
> Cc: John Stultz <john.stultz@...aro.org>
> Suggested-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> Signed-off-by: Yu Chen <chenyu56@...wei.com>

Hey Yu Chen!
   Thanks again for sending this patch out! As mentioned in my
comments with the other patches, I've got one proposal I wanted to
share to try to avoid  state initialization races that I've seen with
this patchset.

> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index f45d8df5cfb8..e2caaa665d6e 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -58,6 +61,20 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_set_role);
>
> +int usb_role_switch_register_notifier(struct usb_role_switch *sw,
> +                                     struct notifier_block *nb)
> +{
> +       return blocking_notifier_chain_register(&sw->nh, nb);
> +}
> +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier);

As noted earlier, one issue I've seen is that the hisi_hikey_usb
driver's notifier may not get called early enough to receive
notification of the initial usb state.

It seems like on registration here, we should take the lock, read the
role state and immediately call the notifier to properly initialize
it? I suspect that should close the window for any state races around
driver probe timings and

Does that make sense? I have roughly prototyped this but need to do
additional testing.

thanks
-john