| Message ID | 20190118033544.3980-1-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated | expand |
On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > In current implementation, we might re-allocate nd_pfn->pfn_sb. > > For example: > > nd_dax_probe() > nd_pfn->pfn_sb = devm_kzalloc() > > dax_pmem_probe() > nvdimm_setup_pfn() > nd_pfn_init() > nd_pfn->pfn_sb = devm_kzalloc() > > This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). Ugh, this patch is unfortunately uncovering a deeper problem. nvdimm_setup_pfn() should not be calling nd_pfn_init(). This effectively is always re-initializing the info-block, benign but unnecessary. Instead it needs to be using nd_pfn_validate() followed by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. Good find!
On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> >> For example: >> >> nd_dax_probe() >> nd_pfn->pfn_sb = devm_kzalloc() >> >> dax_pmem_probe() >> nvdimm_setup_pfn() >> nd_pfn_init() >> nd_pfn->pfn_sb = devm_kzalloc() >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > >Ugh, this patch is unfortunately uncovering a deeper problem. >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >effectively is always re-initializing the info-block, benign but >unnecessary. Instead it needs to be using nd_pfn_validate() followed >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > >Good find! So we should fix this like: dax_pmem_probe() nd_pfn_validate() __nvdimm_setup_pfn() Is my understanding correct?
On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: > >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > >> > >> In current implementation, we might re-allocate nd_pfn->pfn_sb. > >> > >> For example: > >> > >> nd_dax_probe() > >> nd_pfn->pfn_sb = devm_kzalloc() > >> > >> dax_pmem_probe() > >> nvdimm_setup_pfn() > >> nd_pfn_init() > >> nd_pfn->pfn_sb = devm_kzalloc() > >> > >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > > > >Ugh, this patch is unfortunately uncovering a deeper problem. > >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This > >effectively is always re-initializing the info-block, benign but > >unnecessary. Instead it needs to be using nd_pfn_validate() followed > >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > > > >Good find! > > So we should fix this like: > > dax_pmem_probe() > nd_pfn_validate() > __nvdimm_setup_pfn() > > Is my understanding correct? Yes, at first glance, but it would need a deeper review and testing. The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a similar fixup.
On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> >> >> >> For example: >> >> >> >> nd_dax_probe() >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> dax_pmem_probe() >> >> nvdimm_setup_pfn() >> >> nd_pfn_init() >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). >> > >> >Ugh, this patch is unfortunately uncovering a deeper problem. >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >> >effectively is always re-initializing the info-block, benign but >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. >> > >> >Good find! >> >> So we should fix this like: >> >> dax_pmem_probe() >> nd_pfn_validate() >> __nvdimm_setup_pfn() >> >> Is my understanding correct? > >Yes, at first glance, but it would need a deeper review and testing. >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a >similar fixup. I went through the code again and found I may remove the allocation in nd_pfn_init(). Below is my analysis. nvdimm_setup_pfn() is invoked in two places: * dax_pmem_probe() * pmem_attach_disk(), when it is_nd_pfn() And before these two function called, corresponding device should be allocated and created. And we can see these two corresponding places to create the devices are: * nd_dax_probe() * nd_pfn_probe() Both of them allocate pfn_sb. This means it is not necessary to allocate it again when we do the probe function. Do you think it looks good to you?
On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: > >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > >> > >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: > >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > >> >> > >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. > >> >> > >> >> For example: > >> >> > >> >> nd_dax_probe() > >> >> nd_pfn->pfn_sb = devm_kzalloc() > >> >> > >> >> dax_pmem_probe() > >> >> nvdimm_setup_pfn() > >> >> nd_pfn_init() > >> >> nd_pfn->pfn_sb = devm_kzalloc() > >> >> > >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > >> > > >> >Ugh, this patch is unfortunately uncovering a deeper problem. > >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This > >> >effectively is always re-initializing the info-block, benign but > >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed > >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > >> > > >> >Good find! > >> > >> So we should fix this like: > >> > >> dax_pmem_probe() > >> nd_pfn_validate() > >> __nvdimm_setup_pfn() > >> > >> Is my understanding correct? > > > >Yes, at first glance, but it would need a deeper review and testing. > >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a > >similar fixup. > > I went through the code again and found I may remove the allocation in > nd_pfn_init(). > > Below is my analysis. > > nvdimm_setup_pfn() is invoked in two places: > > * dax_pmem_probe() > * pmem_attach_disk(), when it is_nd_pfn() > > And before these two function called, corresponding device should be allocated > and created. And we can see these two corresponding places to create the > devices are: > > * nd_dax_probe() > * nd_pfn_probe() > > Both of them allocate pfn_sb. This means it is not necessary to allocate it > again when we do the probe function. > > Do you think it looks good to you? > No it doesn't because the real issue is that we should not be recreating the info block. So the fix is not avoiding the allocation, the fix is avoiding nd_pfn_init() in the probe path altogether.
On Fri, Jan 18, 2019 at 12:38 AM Dan Williams <dan.j.williams@intel.com> wrote: > > On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > > > On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: > > >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > >> > > >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: > > >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: > > >> >> > > >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. > > >> >> > > >> >> For example: > > >> >> > > >> >> nd_dax_probe() > > >> >> nd_pfn->pfn_sb = devm_kzalloc() > > >> >> > > >> >> dax_pmem_probe() > > >> >> nvdimm_setup_pfn() > > >> >> nd_pfn_init() > > >> >> nd_pfn->pfn_sb = devm_kzalloc() > > >> >> > > >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). > > >> > > > >> >Ugh, this patch is unfortunately uncovering a deeper problem. > > >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This > > >> >effectively is always re-initializing the info-block, benign but > > >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed > > >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. > > >> > > > >> >Good find! > > >> > > >> So we should fix this like: > > >> > > >> dax_pmem_probe() > > >> nd_pfn_validate() > > >> __nvdimm_setup_pfn() > > >> > > >> Is my understanding correct? > > > > > >Yes, at first glance, but it would need a deeper review and testing. > > >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a > > >similar fixup. > > > > I went through the code again and found I may remove the allocation in > > nd_pfn_init(). > > > > Below is my analysis. > > > > nvdimm_setup_pfn() is invoked in two places: > > > > * dax_pmem_probe() > > * pmem_attach_disk(), when it is_nd_pfn() > > > > And before these two function called, corresponding device should be allocated > > and created. And we can see these two corresponding places to create the > > devices are: > > > > * nd_dax_probe() > > * nd_pfn_probe() > > > > Both of them allocate pfn_sb. This means it is not necessary to allocate it > > again when we do the probe function. > > > > Do you think it looks good to you? > > > > No it doesn't because the real issue is that we should not be > recreating the info block. So the fix is not avoiding the allocation, > the fix is avoiding nd_pfn_init() in the probe path altogether. I was wrong, thankfully! nd_pfn_init() *does* call nd_pfn_validate(), I was just overlooking it. I also don't see that this fix is needed. nd_dax_probe() devm-allocates pfn_sb and performs a validation. If the validation succeeds nd_dax_probe() returns 0 which creates a new device and then causes nd_pmem_probe() to fail. At that point devm frees the pfn_sb allocation. The allocation is also freed if the validation step failed.
On Fri, Jan 18, 2019 at 12:38:18AM -0800, Dan Williams wrote: >On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: >> >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> >> >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >> >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> >> >> >> >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> >> >> >> >> >> For example: >> >> >> >> >> >> nd_dax_probe() >> >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> >> >> dax_pmem_probe() >> >> >> nvdimm_setup_pfn() >> >> >> nd_pfn_init() >> >> >> nd_pfn->pfn_sb = devm_kzalloc() >> >> >> >> >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). >> >> > >> >> >Ugh, this patch is unfortunately uncovering a deeper problem. >> >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >> >> >effectively is always re-initializing the info-block, benign but >> >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed >> >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. >> >> > >> >> >Good find! >> >> >> >> So we should fix this like: >> >> >> >> dax_pmem_probe() >> >> nd_pfn_validate() >> >> __nvdimm_setup_pfn() >> >> >> >> Is my understanding correct? >> > >> >Yes, at first glance, but it would need a deeper review and testing. >> >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a >> >similar fixup. >> >> I went through the code again and found I may remove the allocation in >> nd_pfn_init(). >> >> Below is my analysis. >> >> nvdimm_setup_pfn() is invoked in two places: >> >> * dax_pmem_probe() >> * pmem_attach_disk(), when it is_nd_pfn() >> >> And before these two function called, corresponding device should be allocated >> and created. And we can see these two corresponding places to create the >> devices are: >> >> * nd_dax_probe() >> * nd_pfn_probe() >> >> Both of them allocate pfn_sb. This means it is not necessary to allocate it >> again when we do the probe function. >> >> Do you think it looks good to you? >> > >No it doesn't because the real issue is that we should not be >recreating the info block. So the fix is not avoiding the allocation, >the fix is avoiding nd_pfn_init() in the probe path altogether. Hmm... I may lost here. Which info block we recreated? For one particular device type, nvdimm_setup_pfn() is just called once. By preventing re-allocation here is enough to me. I may not get your point here. Would you mind share some light on this place?
On Fri, Jan 18, 2019 at 10:59:23AM -0800, Dan Williams wrote: >On Fri, Jan 18, 2019 at 12:38 AM Dan Williams <dan.j.williams@intel.com> wrote: >> >> On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> > >> > On Thu, Jan 17, 2019 at 09:05:54PM -0800, Dan Williams wrote: >> > >On Thu, Jan 17, 2019 at 9:03 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> > >> >> > >> On Thu, Jan 17, 2019 at 08:31:56PM -0800, Dan Williams wrote: >> > >> >On Thu, Jan 17, 2019 at 7:36 PM Wei Yang <richardw.yang@linux.intel.com> wrote: >> > >> >> >> > >> >> In current implementation, we might re-allocate nd_pfn->pfn_sb. >> > >> >> >> > >> >> For example: >> > >> >> >> > >> >> nd_dax_probe() >> > >> >> nd_pfn->pfn_sb = devm_kzalloc() >> > >> >> >> > >> >> dax_pmem_probe() >> > >> >> nvdimm_setup_pfn() >> > >> >> nd_pfn_init() >> > >> >> nd_pfn->pfn_sb = devm_kzalloc() >> > >> >> >> > >> >> This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). >> > >> > >> > >> >Ugh, this patch is unfortunately uncovering a deeper problem. >> > >> >nvdimm_setup_pfn() should not be calling nd_pfn_init(). This >> > >> >effectively is always re-initializing the info-block, benign but >> > >> >unnecessary. Instead it needs to be using nd_pfn_validate() followed >> > >> >by an __nvdimm_setup_pfn() in the dax_pmem_probe() path. >> > >> > >> > >> >Good find! >> > >> >> > >> So we should fix this like: >> > >> >> > >> dax_pmem_probe() >> > >> nd_pfn_validate() >> > >> __nvdimm_setup_pfn() >> > >> >> > >> Is my understanding correct? >> > > >> > >Yes, at first glance, but it would need a deeper review and testing. >> > >The usage of nvdimm_setup_pfn() in drivers/nvdimm/pmem.c needs a >> > >similar fixup. >> > >> > I went through the code again and found I may remove the allocation in >> > nd_pfn_init(). >> > >> > Below is my analysis. >> > >> > nvdimm_setup_pfn() is invoked in two places: >> > >> > * dax_pmem_probe() >> > * pmem_attach_disk(), when it is_nd_pfn() >> > >> > And before these two function called, corresponding device should be allocated >> > and created. And we can see these two corresponding places to create the >> > devices are: >> > >> > * nd_dax_probe() >> > * nd_pfn_probe() >> > >> > Both of them allocate pfn_sb. This means it is not necessary to allocate it >> > again when we do the probe function. >> > >> > Do you think it looks good to you? >> > >> >> No it doesn't because the real issue is that we should not be >> recreating the info block. So the fix is not avoiding the allocation, >> the fix is avoiding nd_pfn_init() in the probe path altogether. > >I was wrong, thankfully! nd_pfn_init() *does* call nd_pfn_validate(), >I was just overlooking it. > >I also don't see that this fix is needed. nd_dax_probe() >devm-allocates pfn_sb and performs a validation. If the validation >succeeds nd_dax_probe() returns 0 which creates a new device and then >causes nd_pmem_probe() to fail. At that point devm frees the pfn_sb If I am correct, nd_dax_probe() creates a device whose driver is dax_pmem_driver. So the probe function is dax_pmem_probe. Based on my test, one more pfn_sb is allocated in dax_pmem_probe and the driver attached to the device successfully. >allocation. The allocation is also freed if the validation step >failed. nd_pmem_probe() will be called on device created by nd_btt_probe() and nd_pfn_probe(), if I am right. So in which case these two driver will fail to to attach the device and release the memory?
On Sun, Jan 20, 2019 at 4:51 PM Wei Yang <richardw.yang@linux.intel.com> wrote: [..] > >I also don't see that this fix is needed. nd_dax_probe() > >devm-allocates pfn_sb and performs a validation. If the validation > >succeeds nd_dax_probe() returns 0 which creates a new device and then > >causes nd_pmem_probe() to fail. At that point devm frees the pfn_sb > > If I am correct, nd_dax_probe() creates a device whose driver is > dax_pmem_driver. So the probe function is dax_pmem_probe. > > Based on my test, one more pfn_sb is allocated in dax_pmem_probe and the > driver attached to the device successfully. > > >allocation. The allocation is also freed if the validation step > >failed. > > nd_pmem_probe() will be called on device created by nd_btt_probe() and > nd_pfn_probe(), if I am right. > > So in which case these two driver will fail to to attach the device and > release the memory? See nd_pmem_probe(): /* if we find a valid info-block we'll come back as that personality */ if (nd_btt_probe(dev, ndns) == 0 || nd_pfn_probe(dev, ndns) == 0 || nd_dax_probe(dev, ndns) == 0) return -ENXIO; ...all devm allocations are released due to that "return -ENXIO".
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c index 6f22272e8d80..1f4f07007483 100644 --- a/drivers/nvdimm/pfn_devs.c +++ b/drivers/nvdimm/pfn_devs.c @@ -687,21 +687,25 @@ static int nd_pfn_init(struct nd_pfn *nd_pfn) u32 dax_label_reserve = is_nd_dax(&nd_pfn->dev) ? SZ_128K : 0; struct nd_namespace_common *ndns = nd_pfn->ndns; struct nd_namespace_io *nsio = to_nd_namespace_io(&ndns->dev); + struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb; resource_size_t start, size; struct nd_region *nd_region; u32 start_pad, end_trunc; - struct nd_pfn_sb *pfn_sb; unsigned long npfns; phys_addr_t offset; const char *sig; u64 checksum; int rc; - pfn_sb = devm_kzalloc(&nd_pfn->dev, sizeof(*pfn_sb), GFP_KERNEL); - if (!pfn_sb) - return -ENOMEM; + if (!pfn_sb) { + pfn_sb = devm_kzalloc(&nd_pfn->dev, + sizeof(*pfn_sb), GFP_KERNEL); + if (!pfn_sb) + return -ENOMEM; + + nd_pfn->pfn_sb = pfn_sb; + } - nd_pfn->pfn_sb = pfn_sb; if (is_nd_dax(&nd_pfn->dev)) sig = DAX_SIG; else
In current implementation, we might re-allocate nd_pfn->pfn_sb. For example: nd_dax_probe() nd_pfn->pfn_sb = devm_kzalloc() dax_pmem_probe() nvdimm_setup_pfn() nd_pfn_init() nd_pfn->pfn_sb = devm_kzalloc() This patch checks nd_pfn->pfn_sb before allocating it in nd_pfn_init(). Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- drivers/nvdimm/pfn_devs.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)