Re: [PATCH] libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated

[Wei Yang <richardw.yang@linux.intel.com>]

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?

-- 
Wei Yang
Help you, Help me
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm