linux-kernel - Re: [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios

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]

Date:   Sat, 23 Apr 2022 10:29:02 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     linux-cxl@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
        Vishal L Verma <vishal.l.verma@...el.com>,
        "Schofield, Alison" <alison.schofield@...el.com>,
        Linux NVDIMM <nvdimm@...ts.linux.dev>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 8/8] nvdimm: Fix firmware activation deadlock scenarios

On Fri, Apr 22, 2022 at 9:28 PM Ira Weiny <ira.weiny@...el.com> wrote:
>
> On Thu, Apr 21, 2022 at 08:33:51AM -0700, Dan Williams wrote:
> > Lockdep reports the following deadlock scenarios for CXL root device
> > power-management, device_prepare(), operations, and device_shutdown()
> > operations for 'nd_region' devices:
> >
> > ---
> >  Chain exists of:
> >    &nvdimm_region_key --> &nvdimm_bus->reconfig_mutex --> system_transition_mutex
> >
> >   Possible unsafe locking scenario:
> >
> >         CPU0                    CPU1
> >         ----                    ----
> >    lock(system_transition_mutex);
> >                                 lock(&nvdimm_bus->reconfig_mutex);
> >                                 lock(system_transition_mutex);
> >    lock(&nvdimm_region_key);
> >
> > --
> >
> >  Chain exists of:
> >    &cxl_nvdimm_bridge_key --> acpi_scan_lock --> &cxl_root_key
> >
> >   Possible unsafe locking scenario:
> >
> >         CPU0                    CPU1
> >         ----                    ----
> >    lock(&cxl_root_key);
> >                                 lock(acpi_scan_lock);
> >                                 lock(&cxl_root_key);
> >    lock(&cxl_nvdimm_bridge_key);
> >
> > ---
> >
> > These stem from holding nvdimm_bus_lock() over hibernate_quiet_exec()
> > which walks the entire system device topology taking device_lock() along
> > the way. The nvdimm_bus_lock() is protecting against unregistration,
> > multiple simultaneous ops callers, and preventing activate_show() from
> > racing activate_store(). For the first 2, the lock is redundant.
> > Unregistration already flushes all ops users, and sysfs already prevents
> > multiple threads to be active in an ops handler at the same time. For
> > the last userspace should already be waiting for its last
> > activate_store() to complete, and does not need activate_show() to flush
> > the write side, so this lock usage can be deleted in these attributes.
> >
>
> I'm sorry if this is obvious but why can't the locking be removed from
> capability_show() and nvdimm_bus_firmware_visible() as well?

It can, that's a good catch, thanks.