linux-kernel - Re: [PATCH v2-UPDATE2 3/4] resource: Add device-managed insert/remove

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:	Tue, 8 Mar 2016 12:59:44 -0800
From:	Dan Williams <dan.j.williams@...el.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Toshi Kani <toshi.kani@....com>, Ingo Molnar <mingo@...nel.org>,
	Borislav Petkov <bp@...e.de>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
	Linux ACPI <linux-acpi@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2-UPDATE2 3/4] resource: Add device-managed insert/remove_resource()

On Tue, Mar 8, 2016 at 12:05 PM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
> On Tue, Mar 8, 2016 at 10:08 AM, Toshi Kani <toshi.kani@....com> wrote:
>>
>> Add device-managed implementaions of insert_resource() and
>> remove_resouce() functions.
>
> Can we get an example of the users and show how much this would actually help?
>
> insert_resource() is fairly unusual to begin with, and it should
> basically *never* be used by a driver at all (it's more of a bus
> thing).
>
> The patch doesn't look _wrong_, but it does look potentially largely
> pointless. Adding new interfaces to do things that aren't common
> enough to worry about is counter-productive, imho.

Here's the usage patch from Toshi [1] (copied below).  It is indeed a
resource injected by nfit / nvdimm bus implementation.  We just happen
to support nfit and libnvdimm as modules.

The goal of these patches is to use the ACPI NFIT data to create a
"Persistent Memory" rather than "reserved" resource.  This is for
platform-firmware implementations that use E820-Type2 rather than
E820-Type7 to describe pmem.

[1]: https://patchwork.kernel.org/patch/8485691/

8<--
From: Toshi Kani <toshi.kani@....com>

ACPI 6.0 defines persistent memory (PMEM) ranges in multiple
firmware interfaces, e820, EFI, and ACPI NFIT table.  This EFI
change, however, leads to hit a bug in the grub bootloader, which
treats EFI_PERSISTENT_MEMORY type as regular memory and corrupts
stored user data [1].

Therefore, BIOS may set generic reserved type in e820 and EFI
to cover PMEM ranges.  The kernel can initialize PMEM ranges
from ACPI NFIT table alone.

This scheme causes a problem in the iomem table, though.  On x86,
for instance, e820_reserve_resources() initializes top-level entries
(iomem_resource.child) from the e820 table at early boot-time.
This creates "reserved" entry for a PMEM range, which does not allow
region_intersects() to check with PMEM type.

Change acpi_nfit_register_region() to call acpi_nfit_insert_resource(),
which calls devm_insert_resource() to insert a PMEM entry from NFIT
when the iomem table does not have a PMEM entry already.  That is,
when a PMEM range is marked as reserved type in e820, it inserts
"Persistent Memory" entry, which results as follows.

 + "Persistent Memory"
    + "reserved"

This allows the EINJ driver, which calls region_intersects() to
check PMEM ranges, to work continuously even if BIOS sets reserved
type (or sets nothing) to PMEM ranges in e820 and EFI.

[1]: https://lists.gnu.org/archive/html/grub-devel/2015-11/msg00209.html
Signed-off-by: Toshi Kani <toshi.kani@....com>
Cc: Rafael J. Wysocki <rjw@...ysocki.net>
Cc: Dan Williams <dan.j.williams@...el.com>
Cc: Ingo Molnar <mingo@...nel.org>
Cc: Borislav Petkov <bp@...e.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>
---
 drivers/acpi/nfit.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
index fb53db1..d97b53f 100644
--- a/drivers/acpi/nfit.c
+++ b/drivers/acpi/nfit.c
@@ -1571,6 +1571,30 @@ static int ars_status_process_records(struct
nvdimm_bus *nvdimm_bus,
        return 0;
 }

+static int acpi_nfit_insert_resource(struct acpi_nfit_desc *acpi_desc,
+               struct nd_region_desc *ndr_desc)
+{
+       struct resource *res, *nd_res = ndr_desc->res;
+       size_t size = nd_res->end - nd_res->start + 1;
+
+       /* No operation if the region is already registered as PMEM */
+       if (region_intersects(nd_res->start, size, IORESOURCE_MEM,
+                       IORES_DESC_PERSISTENT_MEMORY) == REGION_INTERSECTS)
+               return 0;
+
+       res = devm_kzalloc(acpi_desc->dev, sizeof(*res), GFP_KERNEL);
+       if (!res)
+               return -ENOMEM;
+
+       res->name = "Persistent Memory";
+       res->start = nd_res->start;
+       res->end = nd_res->end;
+       res->flags = IORESOURCE_MEM;
+       res->desc = IORES_DESC_PERSISTENT_MEMORY;
+
+       return devm_insert_resource(acpi_desc->dev, &iomem_resource, res);
+}
+
 static int acpi_nfit_find_poison(struct acpi_nfit_desc *acpi_desc,
                struct nd_region_desc *ndr_desc)
 {
@@ -1781,6 +1805,12 @@ static int acpi_nfit_register_region(struct
acpi_nfit_desc *acpi_desc,

        nvdimm_bus = acpi_desc->nvdimm_bus;
        if (nfit_spa_type(spa) == NFIT_SPA_PM) {
+               rc = acpi_nfit_insert_resource(acpi_desc, ndr_desc);
+               if (rc)
+                       dev_warn(acpi_desc->dev,
+                               "failed to insert pmem resource to iomem: %d\n",
+                               rc);
+
                rc = acpi_nfit_find_poison(acpi_desc, ndr_desc);
                if (rc) {
                        dev_err(acpi_desc->dev,