linux-kernel - Re: [PATCH] libnvdimm: Clarify nd_pfn

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: <20190122002915.GA5984@richard>
Date:   Tue, 22 Jan 2019 08:29:16 +0800
From:   Wei Yang <richardw.yang@...ux.intel.com>
To:     Wei Yang <richardw.yang@...ux.intel.com>
Cc:     Dan Williams <dan.j.williams@...el.com>, linux-nvdimm@...ts.01.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] libnvdimm: Clarify nd_pfn_init() flow

On Mon, Jan 21, 2019 at 03:51:02PM +0800, Wei Yang wrote:
>On Fri, Jan 18, 2019 at 04:47:23PM -0800, Dan Williams wrote:
>>In recent days, 2 engineers, including the original author of
>>nd_pfn_init(), overlooked the internal call to nd_pfn_validate() and the
>>implications to memory allocation.
>>
>>Clarify this situation to help anyone that reads through this code in
>>the future.
>>
>>Reported-by: Wei Yang <richardw.yang@...ux.intel.com>
>>Signed-off-by: Dan Williams <dan.j.williams@...el.com>
>>---
>> drivers/nvdimm/btt_devs.c |    5 +++++
>> drivers/nvdimm/dax_devs.c |    5 +++++
>> drivers/nvdimm/pfn_devs.c |   21 +++++++++++++++++++++
>> 3 files changed, 31 insertions(+)
>>
>>diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
>>index 795ad4ff35ca..e0a6f2491e57 100644
>>--- a/drivers/nvdimm/btt_devs.c
>>+++ b/drivers/nvdimm/btt_devs.c
>>@@ -354,6 +354,11 @@ int nd_btt_probe(struct device *dev, struct nd_namespace_common *ndns)
>> 		put_device(btt_dev);
>> 	}
>> 
>>+	/*
>>+	 * Successful probe indicates to the caller that an nd_btt
>>+	 * personality device has been registered and the caller can
>>+	 * fail the probe of the baseline namespace device.
>>+	 */
>> 	return rc;
>> }
>> EXPORT_SYMBOL(nd_btt_probe);
>>diff --git a/drivers/nvdimm/dax_devs.c b/drivers/nvdimm/dax_devs.c
>>index 0453f49dc708..65010d955fa7 100644
>>--- a/drivers/nvdimm/dax_devs.c
>>+++ b/drivers/nvdimm/dax_devs.c
>>@@ -136,6 +136,11 @@ int nd_dax_probe(struct device *dev, struct nd_namespace_common *ndns)
>> 	} else
>> 		__nd_device_register(dax_dev);
>> 
>>+	/*
>>+	 * Successful probe indicates to the caller that a device-dax
>>+	 * personality device has been registered and the caller can
>>+	 * fail the probe of the baseline namespace device.
>>+	 */
>> 	return rc;
>> }
>> EXPORT_SYMBOL(nd_dax_probe);
>>diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
>>index 6f22272e8d80..a8783b5a76ba 100644
>>--- a/drivers/nvdimm/pfn_devs.c
>>+++ b/drivers/nvdimm/pfn_devs.c
>>@@ -576,6 +576,11 @@ int nd_pfn_probe(struct device *dev, struct nd_namespace_common *ndns)
>> 	} else
>> 		__nd_device_register(pfn_dev);
>> 
>>+	/*
>>+	 * Successful probe indicates to the caller that an nd_pfn
>>+	 * personality device has been registered and the caller can
>>+	 * fail the probe of the baseline namespace device.
>>+	 */
>> 	return rc;
>> }
>> EXPORT_SYMBOL(nd_pfn_probe);
>>@@ -706,6 +711,22 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn)
>> 		sig = DAX_SIG;
>> 	else
>> 		sig = PFN_SIG;
>>+
>>+	/*
>>+	 * Check for an existing 'pfn' superblock before writing a new
>>+	 * one. The intended flow is that on the first probe of an
>>+	 * nd_{pfn,dax} device the superblock is calculated and written
>>+	 * to the namespace. In this case nd_pfn_validate() returns
>>+	 * -ENODEV because no valid superblock exists currently.
>>+	 *
>>+	 * On subsequent probes nd_pfn_validate() will find a valid
>>+	 * superblock and return 0.
>>+	 *
>>+	 * If an assumption of the superblock has been violated, like a
>>+	 * change to the physical alignment of the namespace,
>>+	 * nd_pfn_validate() will return an error other than -ENODEV to
>>+	 * fail probing.
>>+	 */
>
>Let me reply in this thread. Sorry for my poor understand, I don't get it
>clearly now.
>
>To be honest, the structure is a little bit complicated, if my understanding
>is not correct, please forgive my poor understand.
>
>Below is a code flow. To simply analysis, I setup kernel parameter memmap to
>emulate, and configure one namespace to mode devdax. So that we would have the
>same root for code flow.
>
>Let's start with nd_region_driver:
>
>    nd_region_probe
>        nd_region_register_namespaces
>	    create_namespaces
>	nd_region->btt_seed = nd_btt_create(nd_region);
>	nd_region->pfn_seed = nd_pfn_create(nd_region);
>	nd_region->dax_seed = nd_dax_create(nd_region);

May I ask a question about the purpose to create these three device here?

I see nd_pfn_create() doesn't allocate pfn_sb here, and the probe on these
devices failed. Confused why we need these three devices.

-- 
Wei Yang
Help you, Help me