linux-kernel - RE: [PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list

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: <PU1P153MB01694FCAF062E089059CD441BF920@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
Date:   Fri, 1 Feb 2019 23:17:19 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     Dan Williams <dan.j.williams@...el.com>
CC:     Ross Zwisler <zwisler@...nel.org>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Dave Jiang <dave.jiang@...el.com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        Josh Poulson <jopoulso@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "driverdev-devel@...uxdriverproject.org" 
        <driverdev-devel@...uxdriverproject.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Michael Kelley <mikelley@...rosoft.com>,
        Sasha Levin <Alexander.Levin@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>
Subject: RE: [PATCH v2] nfit: add Hyper-V NVDIMM DSM command set to white list

> From: Dan Williams <dan.j.williams@...el.com>
> Sent: Friday, February 1, 2019 9:29 AM
> > Hi Dan,
> > Unluckily it looks this commit causes a regression ...
> > With the patch, "ndctl list" shows nothing, and /dev/pmem0 can't appear.
> > If I revert the patch, it will be back to normal.
> >
> > I attached the config/logs. In the bad case, "dmesg" shows a line
> > [    5.259017] nd_pmem namespace0.0: 0x0000000000000000, too small
> must be at least 0x1000
> > Any idea why this happens? I'm digging into the details and I appreciate your
> insights.
> 
> Looks like it is working as expected. 

I was working on linux-next tree's next-20190107 and this patch did "work fine"
there. The "regression" happens on djbw/nvdimm.git tree's libnvdimm-pending
branch because we have this recent commit (Jan 19):

11189c1089da ("acpi/nfit: Fix command-supported detection"), which makes such
a change in acpi_nfit_ctl():

-       if (!test_bit(cmd, &cmd_mask) || !test_bit(func, &dsm_mask))
+       if (cmd == ND_CMD_CALL && !test_bit(func, &dsm_mask))
+               return -ENOTTY;
+       else if (!test_bit(cmd, &cmd_mask))
                return -ENOTTY;

So previously ND_CMD_GET_CONFIG_DATA fails with -ENOTTY and we're good.

Now the command succeeds, but it looks the returned data is inavlid, and I see
the "regression".

> The regression you are seeing is the fact that the patch enables the kernel to
> enable nvdimm-namespace-label reads. 
Yes.

> Those reads find a namespace index block
> and a label. Unfortunately the label has the LOCAL flag set and Linux
> explicitly ignores pmem namespace labels with that bit set. The reason
Can you please point out the function that ignores the flag?

I checked where NSLABEL_FLAG_LOCAL is used, but it looks I can't find a
related function.

> for that is due to the fact that the original definition of the LOCAL
> bit from v1.1 of the namespace label implementation [1] explicitly
> limited the LOCAL flag to "block aperture" regions. If you clear that
> LOCAL flag I expect it will work. To my knowledge Windows pretends
> that the v1.1 definition never existed.
I'm trying to find out where the flag is used and how to clear it.

> The UEFI 2.7 specification for v1.2 labels states that setting the
> LOCAL flag is optional when "nlabel", number of labels in the set, is
> 1. Linux makes that mandatory as LOCAL is redundant when nlabel is 1.
> 
> That said, the Robustness Principle makes a case that Linux should
> tolerate the bit being set. However, it's just a non-trivial amount of
> work to unwind the ingrained block-aperture assumptions of that bit.
Can you please explain this a bit more? Sorry, I'm new to this area...

Thanks,
-- Dexuan