linux-kernel - Re: [PATCH v1] driver core: Fix device_pm

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: <CAJZ5v0j+pwiaSbAuhyEYbzqda5f7HRf9+AmEC6hOTqLp5XcxmQ@mail.gmail.com>
Date:   Tue, 1 Sep 2020 14:17:22 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Saravana Kannan <saravanak@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Dong Aisheng <dongas86@...il.com>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] driver core: Fix device_pm_lock() locking for device links

On Tue, Sep 1, 2020 at 12:10 AM Saravana Kannan <saravanak@...gle.com> wrote:
>
> This commit fixes two issues:
>
> 1. The lockdep warning reported by Dong Aisheng <dongas86@...il.com> [1].
>
> It is a warning about a cycle (dpm_list_mtx --> kn->active#3 --> fw_lock)
> that was introduced when device-link devices were added to expose device
> link information in sysfs.
>
> The patch that "introduced" this cycle can't be reverted because it's fixes
> a real SRCU issue and also ensures that the device-link device is deleted
> as soon as the device-link is deleted. This is important to avoid sysfs
> name collisions if the device-link is create again immediately (this can
> happen a lot with deferred probing).
>
> 2. device_link_drop_managed() is not grabbing device_pm_lock().
>
> When device_link_del() calls __device_link_del() (device_link_del() ->
> device_link_put_kref() kref_put() -> __device_link_del()) it grabs the
> device_pm_lock().
>
> However, when device_link_drop_managed() calls __device_link_del()
> (device_link_drop_managed() -> kref_put() -> __device_link_del()) it
> doesn't grab device_pm_lock(). There's nothing special about managed
> device-links that remove the need for grabbing device_pm_lock(). So, this
> patch makes sure device_pm_lock() is always held when deleting managed
> links.
>
> And thanks to Stephen Boyd for helping me understand the lockdep splat.
>
> Fixes: 843e600b8a2b ("driver core: Fix sleeping in invalid context during device link deletion")
> Fixes: 515db266a9da ("driver core: Remove device link creation limitation")
> [1] - https://lore.kernel.org/lkml/CAA+hA=S4eAreb7vo69LAXSk2t5=DEKNxHaiY1wSpk4xTp9urLg@mail.gmail.com/
> Reported-by: Dong Aisheng <dongas86@...il.com>
> Signed-off-by: Saravana Kannan <saravanak@...gle.com>
> ---
>
> Rafael,
>
> A bigger question I had is why we need to grab device_pm_lock() around
> device_link_del() in the first place. I understand the need to grab it
> during device_link_add() -- it's because we are checking the supplier is
> in the dpm_list and because we are reordering devices on the dpm_list.
>
> But during deletion, we don't need to do either one of those.  So, why
> do we even need to grab the device_pm_lock() in the first place.

It is not strictly necessary AFAICS.

> The device_links_write_lock() that we already grab before deleting a device
> link seems like it'd be sufficient. If you agree we don't need to grab
> device_pm_lock() during deletion, then I can change this patch to just
> delete that locking.

Yes, please.

Thanks!