On Fri, Jan 18, 2019 at 10:59:23AM -0800, Dan Williams wrote: >On Fri, Jan 18, 2019 at 12:38 AM Dan Williams <[email protected]> wrote: >> >> On Thu, Jan 17, 2019 at 11:31 PM Wei Yang <[email protected]> >> 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 <[email protected]> >> > >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 >> > >> ><[email protected]> 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? -- Wei Yang Help you, Help me _______________________________________________ Linux-nvdimm mailing list [email protected] https://lists.01.org/mailman/listinfo/linux-nvdimm
