[PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

[PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

[Dan Williams]

Currently, attempts to shutdown and re-enable a device-dax instance
trigger:

    Missing reference count teardown definition
    WARNING: CPU: 37 PID: 1608 at mm/memremap.c:211 devm_memremap_pages+0x234/0x850
    [..]
    RIP: 0010:devm_memremap_pages+0x234/0x850
    [..]
    Call Trace:
     dev_dax_probe+0x66/0x190 [device_dax]
     really_probe+0xef/0x390
     driver_probe_device+0xb4/0x100
     device_driver_attach+0x4f/0x60

Given that the setup path initializes pgmap->ref, arrange for it to be
also torn down so devm_memremap_pages() is ready to be called again and
not be mistaken for the 3rd-party per-cpu-ref case.

Fixes: 24917f6b1041 ("memremap: provide an optional internal refcount in struct dev_pagemap")
Reported-by: Fan Du <fan.du@intel.com>
Tested-by: Vishal Verma <vishal.l.verma@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---

Andrew, I have another dax fix pending, so I'm ok to take this through
the nvdimm tree, holler if you want it in -mm.

 mm/memremap.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/memremap.c b/mm/memremap.c
index 6ee03a816d67..86432650f829 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -91,6 +91,12 @@ static void dev_pagemap_cleanup(struct dev_pagemap *pgmap)
 		wait_for_completion(&pgmap->done);
 		percpu_ref_exit(pgmap->ref);
 	}
+	/*
+	 * Undo the pgmap ref assignment for the internal case as the
+	 * caller may re-enable the same pgmap.
+	 */
+	if (pgmap->ref == &pgmap->internal_ref)
+		pgmap->ref = NULL;
 }
 
 static void devm_memremap_pages_release(void *data)

Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

[Jeff Moyer]

Dan Williams <dan.j.williams@intel.com> writes:

> Currently, attempts to shutdown and re-enable a device-dax instance
> trigger:

What does "shutdown and re-enable" translate to?  If I disable and
re-enable a device-dax namespace, I don't see this behavior.

-Jeff

>
>     Missing reference count teardown definition
>     WARNING: CPU: 37 PID: 1608 at mm/memremap.c:211 devm_memremap_pages+0x234/0x850
>     [..]
>     RIP: 0010:devm_memremap_pages+0x234/0x850
>     [..]
>     Call Trace:
>      dev_dax_probe+0x66/0x190 [device_dax]
>      really_probe+0xef/0x390
>      driver_probe_device+0xb4/0x100
>      device_driver_attach+0x4f/0x60
>
> Given that the setup path initializes pgmap->ref, arrange for it to be
> also torn down so devm_memremap_pages() is ready to be called again and
> not be mistaken for the 3rd-party per-cpu-ref case.
>
> Fixes: 24917f6b1041 ("memremap: provide an optional internal refcount in struct dev_pagemap")
> Reported-by: Fan Du <fan.du@intel.com>
> Tested-by: Vishal Verma <vishal.l.verma@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Jason Gunthorpe <jgg@mellanox.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>
> Andrew, I have another dax fix pending, so I'm ok to take this through
> the nvdimm tree, holler if you want it in -mm.
>
>  mm/memremap.c |    6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 6ee03a816d67..86432650f829 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -91,6 +91,12 @@ static void dev_pagemap_cleanup(struct dev_pagemap *pgmap)
>  		wait_for_completion(&pgmap->done);
>  		percpu_ref_exit(pgmap->ref);
>  	}
> +	/*
> +	 * Undo the pgmap ref assignment for the internal case as the
> +	 * caller may re-enable the same pgmap.
> +	 */
> +	if (pgmap->ref == &pgmap->internal_ref)
> +		pgmap->ref = NULL;
>  }
>  
>  static void devm_memremap_pages_release(void *data)
>
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

[Dan Williams]

On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > Currently, attempts to shutdown and re-enable a device-dax instance
> > trigger:
>
> What does "shutdown and re-enable" translate to?  If I disable and
> re-enable a device-dax namespace, I don't see this behavior.

I was not seeing this either until I made sure I was in 'bus" device model mode.

# cat /etc/modprobe.d/daxctl.conf
blacklist dax_pmem_compat
alias nd:t7* dax_pmem

# make TESTS="daxctl-devices.sh" check -j 40 2>out

# dmesg | grep WARN.*devm
[  225.588651] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
devm_memremap_pages+0x234/0x850
[  225.679828] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
devm_memremap_pages+0x234/0x850

Re: [PATCH] mm/memremap: Fix reuse of pgmap instances with internal references

[Jeff Moyer]

Dan Williams <dan.j.williams@intel.com> writes:

> On Mon, Aug 12, 2019 at 8:51 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > Currently, attempts to shutdown and re-enable a device-dax instance
>> > trigger:
>>
>> What does "shutdown and re-enable" translate to?  If I disable and
>> re-enable a device-dax namespace, I don't see this behavior.
>
> I was not seeing this either until I made sure I was in 'bus" device model mode.
>
> # cat /etc/modprobe.d/daxctl.conf
> blacklist dax_pmem_compat
> alias nd:t7* dax_pmem
>
> # make TESTS="daxctl-devices.sh" check -j 40 2>out
>
> # dmesg | grep WARN.*devm
> [  225.588651] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
> devm_memremap_pages+0x234/0x850
> [  225.679828] WARNING: CPU: 10 PID: 9103 at mm/memremap.c:211
> devm_memremap_pages+0x234/0x850

Ah, you see this when reconfiguring the device.  So, the lifetime of the
pgmap is tied to the character device, which doesn't get torn down.  The
fix looks good to me, and tests out fine.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>