libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated
diff mbox series

Message ID 20190118033544.3980-1-richardw.yang@linux.intel.com
State New
Headers show
Series
  • libnvdimm, pfn: not allocate nd_pfn->pfn_sb if already allocated
Related show

Commit Message

Wei Yang Jan. 18, 2019, 3:35 a.m. UTC
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(-)

Comments

Dan Williams Jan. 18, 2019, 4:31 a.m. UTC | #1
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!
Wei Yang Jan. 18, 2019, 5:03 a.m. UTC | #2
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?
Dan Williams Jan. 18, 2019, 5:05 a.m. UTC | #3
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.
Wei Yang Jan. 18, 2019, 7:31 a.m. UTC | #4
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?
Dan Williams Jan. 18, 2019, 8:38 a.m. UTC | #5
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.
Dan Williams Jan. 18, 2019, 6:59 p.m. UTC | #6
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.
Wei Yang Jan. 21, 2019, 12:24 a.m. UTC | #7
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?
Wei Yang Jan. 21, 2019, 12:50 a.m. UTC | #8
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?
Dan Williams Jan. 21, 2019, 3:28 a.m. UTC | #9
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".

Patch
diff mbox series

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