| Message ID | 20240131162533.247710-3-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce cpu_dcache_is_aliasing() to fix DAX regression | expand |
Mathieu Desnoyers wrote: > Replace the following fs/Kconfig:FS_DAX dependency: > > depends on !(ARM || MIPS || SPARC) > > By a runtime check within alloc_dax(). > > This is done in preparation for its use by each filesystem supporting > the "dax" mount option to validate whether DAX is indeed supported. > > This is done in preparation for using cpu_dcache_is_aliasing() in a > following change which will properly support architectures which detect > data cache aliasing at runtime. > > Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches") > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: linux-mm@kvack.org > Cc: linux-arch@vger.kernel.org > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Cc: Dave Jiang <dave.jiang@intel.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Russell King <linux@armlinux.org.uk> > Cc: nvdimm@lists.linux.dev > Cc: linux-cxl@vger.kernel.org > Cc: linux-fsdevel@vger.kernel.org > Cc: dm-devel@lists.linux.dev > --- > drivers/dax/super.c | 6 ++++++ > fs/Kconfig | 1 - > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/dax/super.c b/drivers/dax/super.c > index 0da9232ea175..e9f397b8a5a3 100644 > --- a/drivers/dax/super.c > +++ b/drivers/dax/super.c > @@ -445,6 +445,12 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) > dev_t devt; > int minor; > > + /* Unavailable on architectures with virtually aliased data caches. */ > + if (IS_ENABLED(CONFIG_ARM) || > + IS_ENABLED(CONFIG_MIPS) || > + IS_ENABLED(CONFIG_SPARC)) > + return NULL; This function returns ERR_PTR(), not NULL on failure. ...and I notice this mistake is also made in include/linux/dax.h in the CONFIG_DAX=n case. That function also mentions: static inline struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) { /* * Callers should check IS_ENABLED(CONFIG_DAX) to know if this * NULL is an error or expected. */ return NULL; } ...and none of the callers validate the result, but now runtime validation is necessary. I.e. it is not enough to check IS_ENABLED(CONFIG_DAX) it also needs to check cpu_dcache_is_aliasing(). With that, there are a few more fixup places needed, pmem_attach_disk(), dcssblk_add_store(), and virtio_fs_setup_dax().
On 2024-01-31 16:02, Dan Williams wrote: > Mathieu Desnoyers wrote: >> Replace the following fs/Kconfig:FS_DAX dependency: >> >> depends on !(ARM || MIPS || SPARC) >> >> By a runtime check within alloc_dax(). >> >> This is done in preparation for its use by each filesystem supporting >> the "dax" mount option to validate whether DAX is indeed supported. >> >> This is done in preparation for using cpu_dcache_is_aliasing() in a >> following change which will properly support architectures which detect >> data cache aliasing at runtime. >> >> Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches") >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Linus Torvalds <torvalds@linux-foundation.org> >> Cc: linux-mm@kvack.org >> Cc: linux-arch@vger.kernel.org >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> Cc: Dave Jiang <dave.jiang@intel.com> >> Cc: Matthew Wilcox <willy@infradead.org> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Cc: Russell King <linux@armlinux.org.uk> >> Cc: nvdimm@lists.linux.dev >> Cc: linux-cxl@vger.kernel.org >> Cc: linux-fsdevel@vger.kernel.org >> Cc: dm-devel@lists.linux.dev >> --- >> drivers/dax/super.c | 6 ++++++ >> fs/Kconfig | 1 - >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/dax/super.c b/drivers/dax/super.c >> index 0da9232ea175..e9f397b8a5a3 100644 >> --- a/drivers/dax/super.c >> +++ b/drivers/dax/super.c >> @@ -445,6 +445,12 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) >> dev_t devt; >> int minor; >> >> + /* Unavailable on architectures with virtually aliased data caches. */ >> + if (IS_ENABLED(CONFIG_ARM) || >> + IS_ENABLED(CONFIG_MIPS) || >> + IS_ENABLED(CONFIG_SPARC)) >> + return NULL; > > This function returns ERR_PTR(), not NULL on failure. Except that it returns NULL in the CONFIG_DAX=n case as you noticed below. > > ...and I notice this mistake is also made in include/linux/dax.h in the > CONFIG_DAX=n case. That function also mentions: > > static inline struct dax_device *alloc_dax(void *private, > const struct dax_operations *ops) > { > /* > * Callers should check IS_ENABLED(CONFIG_DAX) to know if this > * NULL is an error or expected. > */ > return NULL; > } > > ...and none of the callers validate the result, but now runtime > validation is necessary. I.e. it is not enough to check > IS_ENABLED(CONFIG_DAX) it also needs to check cpu_dcache_is_aliasing(). If the callers select DAX in their Kconfig, then they don't have to explicitly check for IS_ENABLED(CONFIG_DAX). Things change for the introduced runtime check though. > > With that, there are a few more fixup places needed, pmem_attach_disk(), > dcssblk_add_store(), and virtio_fs_setup_dax(). Which approach should we take then ? Should we: A) Keep returning NULL from alloc_dax() for both cpu_dcache_is_aliasing() and CONFIG_DAX=n, and use IS_ERR_OR_NULL() in the caller. If we do this, then the callers need to somehow translate this NULL into a negative error value, or B) Replace this NULL return value in both cases by a ERR_PTR() (which error value should we return ?). I would favor approach B) which appears more robust and introduces fewer changes. If we go for that approach do we still need to change the callers ? Thanks, Mathieu
Mathieu Desnoyers wrote: > On 2024-01-31 16:02, Dan Williams wrote: > > Mathieu Desnoyers wrote: > >> Replace the following fs/Kconfig:FS_DAX dependency: > >> > >> depends on !(ARM || MIPS || SPARC) > >> > >> By a runtime check within alloc_dax(). > >> > >> This is done in preparation for its use by each filesystem supporting > >> the "dax" mount option to validate whether DAX is indeed supported. > >> > >> This is done in preparation for using cpu_dcache_is_aliasing() in a > >> following change which will properly support architectures which detect > >> data cache aliasing at runtime. > >> > >> Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches") > >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > >> Cc: Andrew Morton <akpm@linux-foundation.org> > >> Cc: Linus Torvalds <torvalds@linux-foundation.org> > >> Cc: linux-mm@kvack.org > >> Cc: linux-arch@vger.kernel.org > >> Cc: Dan Williams <dan.j.williams@intel.com> > >> Cc: Vishal Verma <vishal.l.verma@intel.com> > >> Cc: Dave Jiang <dave.jiang@intel.com> > >> Cc: Matthew Wilcox <willy@infradead.org> > >> Cc: Arnd Bergmann <arnd@arndb.de> > >> Cc: Russell King <linux@armlinux.org.uk> > >> Cc: nvdimm@lists.linux.dev > >> Cc: linux-cxl@vger.kernel.org > >> Cc: linux-fsdevel@vger.kernel.org > >> Cc: dm-devel@lists.linux.dev > >> --- > >> drivers/dax/super.c | 6 ++++++ > >> fs/Kconfig | 1 - > >> 2 files changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/dax/super.c b/drivers/dax/super.c > >> index 0da9232ea175..e9f397b8a5a3 100644 > >> --- a/drivers/dax/super.c > >> +++ b/drivers/dax/super.c > >> @@ -445,6 +445,12 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) > >> dev_t devt; > >> int minor; > >> > >> + /* Unavailable on architectures with virtually aliased data caches. */ > >> + if (IS_ENABLED(CONFIG_ARM) || > >> + IS_ENABLED(CONFIG_MIPS) || > >> + IS_ENABLED(CONFIG_SPARC)) > >> + return NULL; > > > > This function returns ERR_PTR(), not NULL on failure. > > Except that it returns NULL in the CONFIG_DAX=n case as you > noticed below. > > > > > ...and I notice this mistake is also made in include/linux/dax.h in the > > CONFIG_DAX=n case. That function also mentions: > > > > static inline struct dax_device *alloc_dax(void *private, > > const struct dax_operations *ops) > > { > > /* > > * Callers should check IS_ENABLED(CONFIG_DAX) to know if this > > * NULL is an error or expected. > > */ > > return NULL; > > } > > > > ...and none of the callers validate the result, but now runtime > > validation is necessary. I.e. it is not enough to check > > IS_ENABLED(CONFIG_DAX) it also needs to check cpu_dcache_is_aliasing(). > > If the callers select DAX in their Kconfig, then they don't have to > explicitly check for IS_ENABLED(CONFIG_DAX). Things change for the > introduced runtime check though. > > > > > With that, there are a few more fixup places needed, pmem_attach_disk(), > > dcssblk_add_store(), and virtio_fs_setup_dax(). > > Which approach should we take then ? Should we: > > A) Keep returning NULL from alloc_dax() for both > cpu_dcache_is_aliasing() and CONFIG_DAX=n, and use IS_ERR_OR_NULL() > in the caller. If we do this, then the callers need to somehow > translate this NULL into a negative error value, or > > B) Replace this NULL return value in both cases by a ERR_PTR() (which > error value should we return ?). > > I would favor approach B) which appears more robust and introduces > fewer changes. If we go for that approach do we still need to change > the callers ? I agree approach B is the way to go, but that still requires these fixups, feel free to steal these hunks and split them into patches: Co-developed-by: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> ...but note they are compile-tested only. They assume that alloc_dax() returns ERR_PTR(-EOPNOTSUPP) when the arch support is missing, and I wrote them quickly so I might have missed something. diff --git a/drivers/dax/super.c b/drivers/dax/super.c index f4b635526345..254d3b1e420e 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -322,7 +322,7 @@ EXPORT_SYMBOL_GPL(dax_alive); */ void kill_dax(struct dax_device *dax_dev) { - if (!dax_dev) + if (IS_ERR_OR_NULL(dax_dev)) return; if (dax_dev->holder_data != NULL) diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 4e8fdcb3f1c8..b69c9e442cf4 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev, dax_dev = alloc_dax(pmem, &pmem_dax_ops); if (IS_ERR(dax_dev)) { rc = PTR_ERR(dax_dev); - goto out; + if (rc != -EOPNOTSUPP) + goto out; + } else { + set_dax_nocache(dax_dev); + set_dax_nomc(dax_dev); + if (is_nvdimm_sync(nd_region)) + set_dax_synchronous(dax_dev); + rc = dax_add_host(dax_dev, disk); + if (rc) + goto out_cleanup_dax; + dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); + pmem->dax_dev = dax_dev; } - set_dax_nocache(dax_dev); - set_dax_nomc(dax_dev); - if (is_nvdimm_sync(nd_region)) - set_dax_synchronous(dax_dev); - rc = dax_add_host(dax_dev, disk); - if (rc) - goto out_cleanup_dax; - dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); - pmem->dax_dev = dax_dev; rc = device_add_disk(dev, disk, pmem_attribute_groups); if (rc) diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index 4b7ecd4fd431..f911e58a24dd 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -681,12 +681,14 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char if (IS_ERR(dev_info->dax_dev)) { rc = PTR_ERR(dev_info->dax_dev); dev_info->dax_dev = NULL; - goto put_dev; + if (rc != -EOPNOTSUPP) + goto put_dev; + } else { + set_dax_synchronous(dev_info->dax_dev); + rc = dax_add_host(dev_info->dax_dev, dev_info->gd); + if (rc) + goto out_dax; } - set_dax_synchronous(dev_info->dax_dev); - rc = dax_add_host(dev_info->dax_dev, dev_info->gd); - if (rc) - goto out_dax; get_device(&dev_info->dev); rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c index 5f1be1da92ce..11053a70f5ab 100644 --- a/fs/fuse/virtio_fs.c +++ b/fs/fuse/virtio_fs.c @@ -16,6 +16,7 @@ #include <linux/fs_context.h> #include <linux/fs_parser.h> #include <linux/highmem.h> +#include <linux/cleanup.h> #include <linux/uio.h> #include "fuse_i.h" @@ -795,8 +796,11 @@ static void virtio_fs_cleanup_dax(void *data) put_dax(dax_dev); } +DEFINE_FREE(cleanup_dax, struct dax_dev *, if (!IS_ERR_OR_NULL(_T)) virtio_fs_cleanup_dax(_T)) + static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) { + struct dax_device *dax_dev __free(cleanup_dax) = NULL; struct virtio_shm_region cache_reg; struct dev_pagemap *pgmap; bool have_cache; @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) if (!IS_ENABLED(CONFIG_FUSE_DAX)) return 0; + dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); + if (IS_ERR(dax_dev)) { + int rc = PTR_ERR(dax_dev); + + if (rc == -EOPNOTSUPP) + return 0; + return rc; + } + /* Get cache region */ have_cache = virtio_get_shm_region(vdev, &cache_reg, (u8)VIRTIO_FS_SHMCAP_ID_CACHE); @@ -849,10 +862,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len 0x%llx\n", __func__, fs->window_kaddr, cache_reg.addr, cache_reg.len); - fs->dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); - if (IS_ERR(fs->dax_dev)) - return PTR_ERR(fs->dax_dev); - + fs->dax_dev = no_free_ptr(dax_dev); return devm_add_action_or_reset(&vdev->dev, virtio_fs_cleanup_dax, fs->dax_dev); }
On 2024-01-31 17:18, Dan Williams wrote: > Mathieu Desnoyers wrote: >> On 2024-01-31 16:02, Dan Williams wrote: >>> Mathieu Desnoyers wrote: >>>> Replace the following fs/Kconfig:FS_DAX dependency: >>>> >>>> depends on !(ARM || MIPS || SPARC) >>>> >>>> By a runtime check within alloc_dax(). >>>> >>>> This is done in preparation for its use by each filesystem supporting >>>> the "dax" mount option to validate whether DAX is indeed supported. >>>> >>>> This is done in preparation for using cpu_dcache_is_aliasing() in a >>>> following change which will properly support architectures which detect >>>> data cache aliasing at runtime. >>>> >>>> Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches") >>>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >>>> Cc: Andrew Morton <akpm@linux-foundation.org> >>>> Cc: Linus Torvalds <torvalds@linux-foundation.org> >>>> Cc: linux-mm@kvack.org >>>> Cc: linux-arch@vger.kernel.org >>>> Cc: Dan Williams <dan.j.williams@intel.com> >>>> Cc: Vishal Verma <vishal.l.verma@intel.com> >>>> Cc: Dave Jiang <dave.jiang@intel.com> >>>> Cc: Matthew Wilcox <willy@infradead.org> >>>> Cc: Arnd Bergmann <arnd@arndb.de> >>>> Cc: Russell King <linux@armlinux.org.uk> >>>> Cc: nvdimm@lists.linux.dev >>>> Cc: linux-cxl@vger.kernel.org >>>> Cc: linux-fsdevel@vger.kernel.org >>>> Cc: dm-devel@lists.linux.dev >>>> --- >>>> drivers/dax/super.c | 6 ++++++ >>>> fs/Kconfig | 1 - >>>> 2 files changed, 6 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/dax/super.c b/drivers/dax/super.c >>>> index 0da9232ea175..e9f397b8a5a3 100644 >>>> --- a/drivers/dax/super.c >>>> +++ b/drivers/dax/super.c >>>> @@ -445,6 +445,12 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) >>>> dev_t devt; >>>> int minor; >>>> >>>> + /* Unavailable on architectures with virtually aliased data caches. */ >>>> + if (IS_ENABLED(CONFIG_ARM) || >>>> + IS_ENABLED(CONFIG_MIPS) || >>>> + IS_ENABLED(CONFIG_SPARC)) >>>> + return NULL; >>> >>> This function returns ERR_PTR(), not NULL on failure. >> >> Except that it returns NULL in the CONFIG_DAX=n case as you >> noticed below. >> >>> >>> ...and I notice this mistake is also made in include/linux/dax.h in the >>> CONFIG_DAX=n case. That function also mentions: >>> >>> static inline struct dax_device *alloc_dax(void *private, >>> const struct dax_operations *ops) >>> { >>> /* >>> * Callers should check IS_ENABLED(CONFIG_DAX) to know if this >>> * NULL is an error or expected. >>> */ >>> return NULL; >>> } >>> >>> ...and none of the callers validate the result, but now runtime >>> validation is necessary. I.e. it is not enough to check >>> IS_ENABLED(CONFIG_DAX) it also needs to check cpu_dcache_is_aliasing(). >> >> If the callers select DAX in their Kconfig, then they don't have to >> explicitly check for IS_ENABLED(CONFIG_DAX). Things change for the >> introduced runtime check though. >> >>> >>> With that, there are a few more fixup places needed, pmem_attach_disk(), >>> dcssblk_add_store(), and virtio_fs_setup_dax(). >> >> Which approach should we take then ? Should we: >> >> A) Keep returning NULL from alloc_dax() for both >> cpu_dcache_is_aliasing() and CONFIG_DAX=n, and use IS_ERR_OR_NULL() >> in the caller. If we do this, then the callers need to somehow >> translate this NULL into a negative error value, or >> >> B) Replace this NULL return value in both cases by a ERR_PTR() (which >> error value should we return ?). >> >> I would favor approach B) which appears more robust and introduces >> fewer changes. If we go for that approach do we still need to change >> the callers ? > > I agree approach B is the way to go, but that still requires these > fixups, feel free to steal these hunks and split them into patches: > > Co-developed-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > ...but note they are compile-tested only. They assume that alloc_dax() > returns ERR_PTR(-EOPNOTSUPP) when the arch support is missing, and I > wrote them quickly so I might have missed something. The change for -EOPNOTSUPP in header and implementation would look like this (more questions below): diff --git a/include/linux/dax.h b/include/linux/dax.h index b463502b16e1..df2d52b8a245 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -86,11 +86,7 @@ static inline void *dax_holder(struct dax_device *dax_dev) static inline struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) { - /* - * Callers should check IS_ENABLED(CONFIG_DAX) to know if this - * NULL is an error or expected. - */ - return NULL; + return ERR_PTR(-EOPNOTSUPP); } static inline void put_dax(struct dax_device *dax_dev) { diff --git a/drivers/dax/super.c b/drivers/dax/super.c index 8c3a6e8e6334..c1cf6f0bbe12 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -448,7 +448,7 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) /* Unavailable on architectures with virtually aliased data caches. */ if (cpu_dcache_is_aliasing()) - return NULL; + return ERR_PTR(-EOPNOTSUPP); if (WARN_ON_ONCE(ops && !ops->zero_page_range)) return ERR_PTR(-EINVAL); > > diff --git a/drivers/dax/super.c b/drivers/dax/super.c > index f4b635526345..254d3b1e420e 100644 > --- a/drivers/dax/super.c > +++ b/drivers/dax/super.c > @@ -322,7 +322,7 @@ EXPORT_SYMBOL_GPL(dax_alive); > */ > void kill_dax(struct dax_device *dax_dev) > { > - if (!dax_dev) > + if (IS_ERR_OR_NULL(dax_dev)) I am tempted to just leave the "if (!dax_dev)" check here, because many other functions of this API are only protected by a NULL pointer check. I would hate to forget to convert one check in this change, and I don't think it simplifies anything. The alternative route I intend to take is to audit all callers of alloc_dax() and make sure they all save the alloc_dax() return value in a struct dax_device * local variable first for the sake of checking for IS_ERR(). This will leave the xyz->dax_dev pointer initialized to NULL in the error case and simplify the rest of error checking. > return; > > if (dax_dev->holder_data != NULL) > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > index 4e8fdcb3f1c8..b69c9e442cf4 100644 > --- a/drivers/nvdimm/pmem.c > +++ b/drivers/nvdimm/pmem.c > @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev, > dax_dev = alloc_dax(pmem, &pmem_dax_ops); > if (IS_ERR(dax_dev)) { > rc = PTR_ERR(dax_dev); > - goto out; > + if (rc != -EOPNOTSUPP) > + goto out; If I compare the before / after this change, if previously pmem_attach_disk() was called in a configuration with FS_DAX=n, it would result in a NULL pointer dereference. This would be an error handling fix all by itself. Do we really want to return successfully if dax is unsupported, or should we return an error here ? > + } else { > + set_dax_nocache(dax_dev); > + set_dax_nomc(dax_dev); > + if (is_nvdimm_sync(nd_region)) > + set_dax_synchronous(dax_dev); > + rc = dax_add_host(dax_dev, disk); > + if (rc) > + goto out_cleanup_dax; > + dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); > + pmem->dax_dev = dax_dev; > } > - set_dax_nocache(dax_dev); > - set_dax_nomc(dax_dev); > - if (is_nvdimm_sync(nd_region)) > - set_dax_synchronous(dax_dev); > - rc = dax_add_host(dax_dev, disk); > - if (rc) > - goto out_cleanup_dax; > - dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); > - pmem->dax_dev = dax_dev; > > rc = device_add_disk(dev, disk, pmem_attribute_groups); > if (rc) > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > index 4b7ecd4fd431..f911e58a24dd 100644 > --- a/drivers/s390/block/dcssblk.c > +++ b/drivers/s390/block/dcssblk.c > @@ -681,12 +681,14 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > if (IS_ERR(dev_info->dax_dev)) { > rc = PTR_ERR(dev_info->dax_dev); > dev_info->dax_dev = NULL; > - goto put_dev; > + if (rc != -EOPNOTSUPP) > + goto put_dev; config DCSSBLK selects FS_DAX_LIMITED and DAX. I'm not sure what selecting DAX is trying to achieve here, because the Kconfig option is "FS_DAX". So depending on the real motivation behind this select, we may want to consider failure rather than success in the -EOPNOTSUPP case. > + } else { > + set_dax_synchronous(dev_info->dax_dev); > + rc = dax_add_host(dev_info->dax_dev, dev_info->gd); > + if (rc) > + goto out_dax; > } > - set_dax_synchronous(dev_info->dax_dev); > - rc = dax_add_host(dev_info->dax_dev, dev_info->gd); > - if (rc) > - goto out_dax; > > get_device(&dev_info->dev); > rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); My own changes, if we want failure on -EOPNOTSUPP: diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c index 4b7ecd4fd431..f363c1d51d9a 100644 --- a/drivers/s390/block/dcssblk.c +++ b/drivers/s390/block/dcssblk.c @@ -549,6 +549,7 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char int rc, i, j, num_of_segments; struct dcssblk_dev_info *dev_info; struct segment_info *seg_info, *temp; + struct dax_device *dax_dev; char *local_buf; unsigned long seg_byte_size; @@ -677,13 +678,13 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char if (rc) goto put_dev; - dev_info->dax_dev = alloc_dax(dev_info, &dcssblk_dax_ops); - if (IS_ERR(dev_info->dax_dev)) { - rc = PTR_ERR(dev_info->dax_dev); - dev_info->dax_dev = NULL; + dax_dev = alloc_dax(dev_info, &dcssblk_dax_ops); + if (IS_ERR(dax_dev)) { + rc = PTR_ERR(dax_dev); goto put_dev; } - set_dax_synchronous(dev_info->dax_dev); + set_dax_synchronous(dax_dev); + dev_info->dax_dev = dax_dev; rc = dax_add_host(dev_info->dax_dev, dev_info->gd); if (rc) goto out_dax; > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > index 5f1be1da92ce..11053a70f5ab 100644 > --- a/fs/fuse/virtio_fs.c > +++ b/fs/fuse/virtio_fs.c > @@ -16,6 +16,7 @@ > #include <linux/fs_context.h> > #include <linux/fs_parser.h> > #include <linux/highmem.h> > +#include <linux/cleanup.h> > #include <linux/uio.h> > #include "fuse_i.h" > > @@ -795,8 +796,11 @@ static void virtio_fs_cleanup_dax(void *data) > put_dax(dax_dev); > } > > +DEFINE_FREE(cleanup_dax, struct dax_dev *, if (!IS_ERR_OR_NULL(_T)) virtio_fs_cleanup_dax(_T)) > + > static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) So either I'm completely missing how ownership works in this function, or we should be really concerned about the fact that it does no actual cleanup of anything on any error. I would be tempted to first refactor this function without using cleanup.h so those fixes can be easily backported to older kernels (?) Here what I'm seeing so far: - devm_release_mem_region() is never called after devm_request_mem_region(). Not on error, neither on teardown, - pgmap is never freed on error after devm_kzalloc. > { > + struct dax_device *dax_dev __free(cleanup_dax) = NULL; > struct virtio_shm_region cache_reg; > struct dev_pagemap *pgmap; > bool have_cache; > @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) > if (!IS_ENABLED(CONFIG_FUSE_DAX)) > return 0; > > + dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); > + if (IS_ERR(dax_dev)) { > + int rc = PTR_ERR(dax_dev); > + > + if (rc == -EOPNOTSUPP) > + return 0; > + return rc; > + } What is gained by moving this allocation here ? > + > /* Get cache region */ > have_cache = virtio_get_shm_region(vdev, &cache_reg, > (u8)VIRTIO_FS_SHMCAP_ID_CACHE); > @@ -849,10 +862,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) > dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len 0x%llx\n", > __func__, fs->window_kaddr, cache_reg.addr, cache_reg.len); > > - fs->dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); > - if (IS_ERR(fs->dax_dev)) > - return PTR_ERR(fs->dax_dev); > - > + fs->dax_dev = no_free_ptr(dax_dev); > return devm_add_action_or_reset(&vdev->dev, virtio_fs_cleanup_dax, > fs->dax_dev); > } In addition I have: diff --git a/drivers/md/dm.c b/drivers/md/dm.c index f90743a94da9..86de91b35f4d 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -2054,6 +2054,7 @@ static void cleanup_mapped_device(struct mapped_device *md) static struct mapped_device *alloc_dev(int minor) { int r, numa_node_id = dm_get_numa_node(); + struct dax_device *dax_dev; struct mapped_device *md; void *old_md; @@ -2122,16 +2123,13 @@ static struct mapped_device *alloc_dev(int minor) md->disk->private_data = md; sprintf(md->disk->disk_name, "dm-%d", minor); - if (IS_ENABLED(CONFIG_FS_DAX)) { - md->dax_dev = alloc_dax(md, &dm_dax_ops); - if (IS_ERR(md->dax_dev)) { - md->dax_dev = NULL; - } else { - set_dax_nocache(md->dax_dev); - set_dax_nomc(md->dax_dev); - if (dax_add_host(md->dax_dev, md->disk)) - goto bad; - } + dax_dev = alloc_dax(md, &dm_dax_ops); + if (!IS_ERR(dax_dev)) { + set_dax_nocache(dax_dev); + set_dax_nomc(dax_dev); + md->dax_dev = dax_dev; + if (dax_add_host(dax_dev, md->disk)) + goto bad; } format_dev_t(md->name, MKDEV(_major, minor)); Thanks, Mathieu
On 2024-02-01 10:44, Mathieu Desnoyers wrote: [...] >> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c >> index 4e8fdcb3f1c8..b69c9e442cf4 100644 >> --- a/drivers/nvdimm/pmem.c >> +++ b/drivers/nvdimm/pmem.c >> @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev, >> dax_dev = alloc_dax(pmem, &pmem_dax_ops); >> if (IS_ERR(dax_dev)) { >> rc = PTR_ERR(dax_dev); >> - goto out; >> + if (rc != -EOPNOTSUPP) >> + goto out; > > If I compare the before / after this change, if previously > pmem_attach_disk() was called in a configuration with FS_DAX=n, it would > result in a NULL pointer dereference. I was wrong. drivers/nvdimm/Kconfig has: config BLK_DEV_PMEM select DAX and drivers/nvdimm/Makefile has: obj-$(CONFIG_BLK_DEV_PMEM) += nd_pmem.o nd_pmem-y := pmem.o which means that anything in pmem.c can assume that alloc_dax() is implemented. [...] >> diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c >> index 4b7ecd4fd431..f911e58a24dd 100644 >> --- a/drivers/s390/block/dcssblk.c >> +++ b/drivers/s390/block/dcssblk.c >> @@ -681,12 +681,14 @@ dcssblk_add_store(struct device *dev, struct >> device_attribute *attr, const char >> if (IS_ERR(dev_info->dax_dev)) { >> rc = PTR_ERR(dev_info->dax_dev); >> dev_info->dax_dev = NULL; >> - goto put_dev; >> + if (rc != -EOPNOTSUPP) >> + goto put_dev; > > config DCSSBLK selects FS_DAX_LIMITED and DAX. > > I'm not sure what selecting DAX is trying to achieve here, because the > Kconfig option is "FS_DAX". > > So depending on the real motivation behind this select, we may want to > consider failure rather than success in the -EOPNOTSUPP case. > I missed that alloc_dax() is implemented as not supported based on CONFIG_DAX (not CONFIG_FS_DAX). Therefore DCSSBLK Kconfig does the right thing and always selects DAX, and thus an implemented version of alloc_dax(). This takes care of two of my open questions at least. :) Thanks, Mathieu
On 2024-02-01 10:44, Mathieu Desnoyers wrote: > On 2024-01-31 17:18, Dan Williams wrote: [...] >> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c >> index 5f1be1da92ce..11053a70f5ab 100644 >> --- a/fs/fuse/virtio_fs.c >> +++ b/fs/fuse/virtio_fs.c >> @@ -16,6 +16,7 @@ >> #include <linux/fs_context.h> >> #include <linux/fs_parser.h> >> #include <linux/highmem.h> >> +#include <linux/cleanup.h> >> #include <linux/uio.h> >> #include "fuse_i.h" >> @@ -795,8 +796,11 @@ static void virtio_fs_cleanup_dax(void *data) >> put_dax(dax_dev); >> } >> +DEFINE_FREE(cleanup_dax, struct dax_dev *, if (!IS_ERR_OR_NULL(_T)) >> virtio_fs_cleanup_dax(_T)) >> + >> static int virtio_fs_setup_dax(struct virtio_device *vdev, struct >> virtio_fs *fs) > > So either I'm completely missing how ownership works in this function, or > we should be really concerned about the fact that it does no actual > cleanup of anything on any error. [...] > > Here what I'm seeing so far: > > - devm_release_mem_region() is never called after > devm_request_mem_region(). Not > on error, neither on teardown, > - pgmap is never freed on error after devm_kzalloc. I was indeed missing something: the devm_ family of functions keeps ownership at the device level, so we would not need explicit teardown. > >> { >> + struct dax_device *dax_dev __free(cleanup_dax) = NULL; >> struct virtio_shm_region cache_reg; >> struct dev_pagemap *pgmap; >> bool have_cache; >> @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct >> virtio_device *vdev, struct virtio_fs *fs) >> if (!IS_ENABLED(CONFIG_FUSE_DAX)) >> return 0; >> + dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); >> + if (IS_ERR(dax_dev)) { >> + int rc = PTR_ERR(dax_dev); >> + >> + if (rc == -EOPNOTSUPP) >> + return 0; >> + return rc; >> + } > > What is gained by moving this allocation here ? I'm still concerned about moving the call to alloc_dax() before the setup of the memory region it will use. Are those completely independent ? > >> + >> /* Get cache region */ >> have_cache = virtio_get_shm_region(vdev, &cache_reg, >> (u8)VIRTIO_FS_SHMCAP_ID_CACHE); >> @@ -849,10 +862,7 @@ static int virtio_fs_setup_dax(struct >> virtio_device *vdev, struct virtio_fs *fs) >> dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len >> 0x%llx\n", >> __func__, fs->window_kaddr, cache_reg.addr, cache_reg.len); >> - fs->dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); >> - if (IS_ERR(fs->dax_dev)) >> - return PTR_ERR(fs->dax_dev); >> - >> + fs->dax_dev = no_free_ptr(dax_dev); >> return devm_add_action_or_reset(&vdev->dev, virtio_fs_cleanup_dax, >> fs->dax_dev); >> } > [...] Thanks, Mathieu
Mathieu Desnoyers wrote: [..] > The change for -EOPNOTSUPP in header and implementation would look like > this (more questions below): > > diff --git a/include/linux/dax.h b/include/linux/dax.h > index b463502b16e1..df2d52b8a245 100644 > --- a/include/linux/dax.h > +++ b/include/linux/dax.h > @@ -86,11 +86,7 @@ static inline void *dax_holder(struct dax_device *dax_dev) > static inline struct dax_device *alloc_dax(void *private, > const struct dax_operations *ops) > { > - /* > - * Callers should check IS_ENABLED(CONFIG_DAX) to know if this > - * NULL is an error or expected. > - */ > - return NULL; > + return ERR_PTR(-EOPNOTSUPP); > } > static inline void put_dax(struct dax_device *dax_dev) > { > diff --git a/drivers/dax/super.c b/drivers/dax/super.c > index 8c3a6e8e6334..c1cf6f0bbe12 100644 > --- a/drivers/dax/super.c > +++ b/drivers/dax/super.c > @@ -448,7 +448,7 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) > > /* Unavailable on architectures with virtually aliased data caches. */ > if (cpu_dcache_is_aliasing()) > - return NULL; > + return ERR_PTR(-EOPNOTSUPP); > > if (WARN_ON_ONCE(ops && !ops->zero_page_range)) > return ERR_PTR(-EINVAL); > > > > > diff --git a/drivers/dax/super.c b/drivers/dax/super.c > > index f4b635526345..254d3b1e420e 100644 > > --- a/drivers/dax/super.c > > +++ b/drivers/dax/super.c > > @@ -322,7 +322,7 @@ EXPORT_SYMBOL_GPL(dax_alive); > > */ > > void kill_dax(struct dax_device *dax_dev) > > { > > - if (!dax_dev) > > + if (IS_ERR_OR_NULL(dax_dev)) > > I am tempted to just leave the "if (!dax_dev)" check here, because > many other functions of this API are only protected by a NULL > pointer check. I would hate to forget to convert one check in this > change, and I don't think it simplifies anything. It's not that it simplifies anything, but mistakes fail safely as a memory leak rather than a crash. > The alternative route I intend to take is to audit all callers > of alloc_dax() and make sure they all save the alloc_dax() return > value in a struct dax_device * local variable first for the sake > of checking for IS_ERR(). This will leave the xyz->dax_dev pointer > initialized to NULL in the error case and simplify the rest of > error checking. I could maybe get on board with that, but it needs a comment somewhere about the asymmetric subtlety. > > > > return; > > > > if (dax_dev->holder_data != NULL) > > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > > index 4e8fdcb3f1c8..b69c9e442cf4 100644 > > --- a/drivers/nvdimm/pmem.c > > +++ b/drivers/nvdimm/pmem.c > > @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev, > > dax_dev = alloc_dax(pmem, &pmem_dax_ops); > > if (IS_ERR(dax_dev)) { > > rc = PTR_ERR(dax_dev); > > - goto out; > > + if (rc != -EOPNOTSUPP) > > + goto out; > > If I compare the before / after this change, if previously > pmem_attach_disk() was called in a configuration with FS_DAX=n, it would > result in a NULL pointer dereference. No, alloc_dax() only returns NULL CONFIG_DAX=n case, not the CONFIG_FS_DAX=n case. So that means that pmem devices on ARM have been possible without FS_DAX. So, in order for alloc_dax() returning ERR_PTR(-EOPNOTSUPP) to not regress pmem device availability this error path needs to be changed. > This would be an error handling fix all by itself. Do we really want > to return successfully if dax is unsupported, or should we return > an error here ? Per above, there is no error handling fix, and pmem block device available should not depend on alloc_dax() succeeding. The real question is what to do about device-dax. I *think* it is not affected by cpu_dcache aliasing because it never accesses user mappings through a kernel alias. I doubt device-dax is in use on these platforms, but we might need another fixup for that if someone screams about the alloc_dax() behavior change making them lose device-dax access. > > + } else { > > + set_dax_nocache(dax_dev); > > + set_dax_nomc(dax_dev); > > + if (is_nvdimm_sync(nd_region)) > > + set_dax_synchronous(dax_dev); > > + rc = dax_add_host(dax_dev, disk); > > + if (rc) > > + goto out_cleanup_dax; > > + dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); > > + pmem->dax_dev = dax_dev; > > } > > - set_dax_nocache(dax_dev); > > - set_dax_nomc(dax_dev); > > - if (is_nvdimm_sync(nd_region)) > > - set_dax_synchronous(dax_dev); > > - rc = dax_add_host(dax_dev, disk); > > - if (rc) > > - goto out_cleanup_dax; > > - dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); > > - pmem->dax_dev = dax_dev; > > > > rc = device_add_disk(dev, disk, pmem_attribute_groups); > > if (rc) > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > > index 4b7ecd4fd431..f911e58a24dd 100644 > > --- a/drivers/s390/block/dcssblk.c > > +++ b/drivers/s390/block/dcssblk.c > > @@ -681,12 +681,14 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > > if (IS_ERR(dev_info->dax_dev)) { > > rc = PTR_ERR(dev_info->dax_dev); > > dev_info->dax_dev = NULL; > > - goto put_dev; > > + if (rc != -EOPNOTSUPP) > > + goto put_dev; > > config DCSSBLK selects FS_DAX_LIMITED and DAX. > > I'm not sure what selecting DAX is trying to achieve here, because the > Kconfig option is "FS_DAX". > > So depending on the real motivation behind this select, we may want to > consider failure rather than success in the -EOPNOTSUPP case. Ah, true, s390 is a !cpu_dcache_is_aliasing() arch, so it is ok to fail driver load on alloc_dax() failure as it knows that ERR_PTR(-EOPNOTSUPP) will never be returned. > > > + } else { > > + set_dax_synchronous(dev_info->dax_dev); > > + rc = dax_add_host(dev_info->dax_dev, dev_info->gd); > > + if (rc) > > + goto out_dax; > > } > > - set_dax_synchronous(dev_info->dax_dev); > > - rc = dax_add_host(dev_info->dax_dev, dev_info->gd); > > - if (rc) > > - goto out_dax; > > > > get_device(&dev_info->dev); > > rc = device_add_disk(&dev_info->dev, dev_info->gd, NULL); > > My own changes, if we want failure on -EOPNOTSUPP: > > diff --git a/drivers/s390/block/dcssblk.c b/drivers/s390/block/dcssblk.c > index 4b7ecd4fd431..f363c1d51d9a 100644 > --- a/drivers/s390/block/dcssblk.c > +++ b/drivers/s390/block/dcssblk.c > @@ -549,6 +549,7 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > int rc, i, j, num_of_segments; > struct dcssblk_dev_info *dev_info; > struct segment_info *seg_info, *temp; > + struct dax_device *dax_dev; > char *local_buf; > unsigned long seg_byte_size; > > @@ -677,13 +678,13 @@ dcssblk_add_store(struct device *dev, struct device_attribute *attr, const char > if (rc) > goto put_dev; > > - dev_info->dax_dev = alloc_dax(dev_info, &dcssblk_dax_ops); > - if (IS_ERR(dev_info->dax_dev)) { > - rc = PTR_ERR(dev_info->dax_dev); > - dev_info->dax_dev = NULL; > + dax_dev = alloc_dax(dev_info, &dcssblk_dax_ops); > + if (IS_ERR(dax_dev)) { > + rc = PTR_ERR(dax_dev); > goto put_dev; > } > - set_dax_synchronous(dev_info->dax_dev); > + set_dax_synchronous(dax_dev); > + dev_info->dax_dev = dax_dev; Looks good to me. > rc = dax_add_host(dev_info->dax_dev, dev_info->gd); > if (rc) > goto out_dax; > > > > diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c > > index 5f1be1da92ce..11053a70f5ab 100644 > > --- a/fs/fuse/virtio_fs.c > > +++ b/fs/fuse/virtio_fs.c > > @@ -16,6 +16,7 @@ > > #include <linux/fs_context.h> > > #include <linux/fs_parser.h> > > #include <linux/highmem.h> > > +#include <linux/cleanup.h> > > #include <linux/uio.h> > > #include "fuse_i.h" > > > > @@ -795,8 +796,11 @@ static void virtio_fs_cleanup_dax(void *data) > > put_dax(dax_dev); > > } > > > > +DEFINE_FREE(cleanup_dax, struct dax_dev *, if (!IS_ERR_OR_NULL(_T)) virtio_fs_cleanup_dax(_T)) > > + > > static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) > > So either I'm completely missing how ownership works in this function, or > we should be really concerned about the fact that it does no actual > cleanup of anything on any error. > > I would be tempted to first refactor this function without using cleanup.h > so those fixes can be easily backported to older kernels (?) > > Here what I'm seeing so far: > > - devm_release_mem_region() is never called after devm_request_mem_region(). Not > on error, neither on teardown, devm_release_mem_region() is called from virtio_fs_probe() context. That means that when virtio_fs_probe() returns an error the driver core will automatically call devm_request_mem_region(). > - pgmap is never freed on error after devm_kzalloc. That is what the "devm_" in devm_kzalloc() does, free the memory on driver-probe failure, or after the driver remove callback is invoked. > > > { > > + struct dax_device *dax_dev __free(cleanup_dax) = NULL; > > struct virtio_shm_region cache_reg; > > struct dev_pagemap *pgmap; > > bool have_cache; > > @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) > > if (!IS_ENABLED(CONFIG_FUSE_DAX)) > > return 0; > > > > + dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); > > + if (IS_ERR(dax_dev)) { > > + int rc = PTR_ERR(dax_dev); > > + > > + if (rc == -EOPNOTSUPP) > > + return 0; > > + return rc; > > + } > > What is gained by moving this allocation here ? The gain is to fail early in virtio_fs_setup_dax() since the fundamental dependency of alloc_dax() success is not met. For example why let the setup progress to devm_memremap_pages() when alloc_dax() is going to return ERR_PTR(-EOPNOTSUPP). > > > + > > /* Get cache region */ > > have_cache = virtio_get_shm_region(vdev, &cache_reg, > > (u8)VIRTIO_FS_SHMCAP_ID_CACHE); > > @@ -849,10 +862,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) > > dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len 0x%llx\n", > > __func__, fs->window_kaddr, cache_reg.addr, cache_reg.len); > > > > - fs->dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); > > - if (IS_ERR(fs->dax_dev)) > > - return PTR_ERR(fs->dax_dev); > > - > > + fs->dax_dev = no_free_ptr(dax_dev); > > return devm_add_action_or_reset(&vdev->dev, virtio_fs_cleanup_dax, > > fs->dax_dev); > > } > > In addition I have: > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index f90743a94da9..86de91b35f4d 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -2054,6 +2054,7 @@ static void cleanup_mapped_device(struct mapped_device *md) > static struct mapped_device *alloc_dev(int minor) > { > int r, numa_node_id = dm_get_numa_node(); > + struct dax_device *dax_dev; > struct mapped_device *md; > void *old_md; > > @@ -2122,16 +2123,13 @@ static struct mapped_device *alloc_dev(int minor) > md->disk->private_data = md; > sprintf(md->disk->disk_name, "dm-%d", minor); > > - if (IS_ENABLED(CONFIG_FS_DAX)) { > - md->dax_dev = alloc_dax(md, &dm_dax_ops); > - if (IS_ERR(md->dax_dev)) { > - md->dax_dev = NULL; > - } else { > - set_dax_nocache(md->dax_dev); > - set_dax_nomc(md->dax_dev); > - if (dax_add_host(md->dax_dev, md->disk)) > - goto bad; > - } > + dax_dev = alloc_dax(md, &dm_dax_ops); > + if (!IS_ERR(dax_dev)) { > + set_dax_nocache(dax_dev); > + set_dax_nomc(dax_dev); > + md->dax_dev = dax_dev; > + if (dax_add_host(dax_dev, md->disk)) > + goto bad; Looks good.
On 2024-02-02 12:37, Dan Williams wrote: > Mathieu Desnoyers wrote: [...] >> > >> The alternative route I intend to take is to audit all callers >> of alloc_dax() and make sure they all save the alloc_dax() return >> value in a struct dax_device * local variable first for the sake >> of checking for IS_ERR(). This will leave the xyz->dax_dev pointer >> initialized to NULL in the error case and simplify the rest of >> error checking. > > I could maybe get on board with that, but it needs a comment somewhere > about the asymmetric subtlety. Is this "somewhere" at every alloc_dax() call site, or do you have something else in mind ? > >> >> >>> return; >>> >>> if (dax_dev->holder_data != NULL) >>> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c >>> index 4e8fdcb3f1c8..b69c9e442cf4 100644 >>> --- a/drivers/nvdimm/pmem.c >>> +++ b/drivers/nvdimm/pmem.c >>> @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev, >>> dax_dev = alloc_dax(pmem, &pmem_dax_ops); >>> if (IS_ERR(dax_dev)) { >>> rc = PTR_ERR(dax_dev); >>> - goto out; >>> + if (rc != -EOPNOTSUPP) >>> + goto out; >> >> If I compare the before / after this change, if previously >> pmem_attach_disk() was called in a configuration with FS_DAX=n, it would >> result in a NULL pointer dereference. > > No, alloc_dax() only returns NULL CONFIG_DAX=n case, not the > CONFIG_FS_DAX=n case. Indeed, I was wrong there. > So that means that pmem devices on ARM have been > possible without FS_DAX. So, in order for alloc_dax() returning > ERR_PTR(-EOPNOTSUPP) to not regress pmem device availability this error > path needs to be changed. Good point. We're moving the depends on !(ARM || MIPS |PARC) from FS_DAX Kconfig to a runtime check in alloc_dax(), which is used whenever DAX=y, which includes configurations that had FS_DAX=n previously. I'll change the error path in pmem_attack_disk to treat -EOPNOTSUPP alloc_dax() return value as non-fatal. > >> This would be an error handling fix all by itself. Do we really want >> to return successfully if dax is unsupported, or should we return >> an error here ? > > Per above, there is no error handling fix, and pmem block device > available should not depend on alloc_dax() succeeding. I agree on treating alloc_dax() failure as non-fatal. There is however one error handling fix to nvdimm/pmem which I plan to introduce as an initial patch before this change: nvdimm/pmem: Fix leak on dax_add_host() failure Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done before setting pmem->dax_dev, which therefore issues the two following calls on NULL pointers: out_cleanup_dax: kill_dax(pmem->dax_dev); put_dax(pmem->dax_dev); Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c index 4e8fdcb3f1c8..9fe358090720 100644 --- a/drivers/nvdimm/pmem.c +++ b/drivers/nvdimm/pmem.c @@ -566,12 +566,11 @@ static int pmem_attach_disk(struct device *dev, set_dax_nomc(dax_dev); if (is_nvdimm_sync(nd_region)) set_dax_synchronous(dax_dev); + pmem->dax_dev = dax_dev; rc = dax_add_host(dax_dev, disk); if (rc) goto out_cleanup_dax; dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); - pmem->dax_dev = dax_dev; - rc = device_add_disk(dev, disk, pmem_attribute_groups); if (rc) goto out_remove_host; > > The real question is what to do about device-dax. I *think* it is not > affected by cpu_dcache aliasing because it never accesses user mappings > through a kernel alias. I doubt device-dax is in use on these platforms, > but we might need another fixup for that if someone screams about the > alloc_dax() behavior change making them lose device-dax access. By "device-dax", I understand you mean drivers/dax/Kconfig:DEV_DAX. Based on your analysis, is alloc_dax() still the right spot where to place this runtime check ? Which call sites are responsible for invoking alloc_dax() for device-dax ? If we know which call sites do not intend to use the kernel linear mapping, we could introduce a flag (or a new variant of the alloc_dax() API) that would either enforce or skip the check. [...] >> >> Here what I'm seeing so far: >> >> - devm_release_mem_region() is never called after devm_request_mem_region(). Not >> on error, neither on teardown, > > devm_release_mem_region() is called from virtio_fs_probe() context. That I guess you mean "devm_request_mem_region()" here. > means that when virtio_fs_probe() returns an error the driver core will > automatically call devm_request_mem_region(). And "devm_release_mem_region()" here. > >> - pgmap is never freed on error after devm_kzalloc. > > That is what the "devm_" in devm_kzalloc() does, free the memory on > driver-probe failure, or after the driver remove callback is invoked. Got it. > >> >>> { >>> + struct dax_device *dax_dev __free(cleanup_dax) = NULL; >>> struct virtio_shm_region cache_reg; >>> struct dev_pagemap *pgmap; >>> bool have_cache; >>> @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) >>> if (!IS_ENABLED(CONFIG_FUSE_DAX)) >>> return 0; >>> >>> + dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); >>> + if (IS_ERR(dax_dev)) { >>> + int rc = PTR_ERR(dax_dev); >>> + >>> + if (rc == -EOPNOTSUPP) >>> + return 0; >>> + return rc; >>> + } >> >> What is gained by moving this allocation here ? > > The gain is to fail early in virtio_fs_setup_dax() since the fundamental > dependency of alloc_dax() success is not met. For example why let the > setup progress to devm_memremap_pages() when alloc_dax() is going to > return ERR_PTR(-EOPNOTSUPP). What I don't know is whether there is a dependency requiring to do devm_request_mem_region(), devm_kzalloc(), devm_memremap_pages() before calling alloc_dax() ? Those 3 calls are used to populate: fs->window_phys_addr = (phys_addr_t) cache_reg.addr; fs->window_len = (phys_addr_t) cache_reg.len; and then alloc_dax() takes "fs" as private data parameter. So it's unclear to me whether we can swap the invocation order. I suspect that it is not an issue because it is only used to populate dax_dev->private, but I prefer to confirm this with you just to be on the safe side. [...] Thanks, Mathieu
Mathieu Desnoyers wrote: > On 2024-02-02 12:37, Dan Williams wrote: > > Mathieu Desnoyers wrote: > [...] > >> > > > >> The alternative route I intend to take is to audit all callers > >> of alloc_dax() and make sure they all save the alloc_dax() return > >> value in a struct dax_device * local variable first for the sake > >> of checking for IS_ERR(). This will leave the xyz->dax_dev pointer > >> initialized to NULL in the error case and simplify the rest of > >> error checking. > > > > I could maybe get on board with that, but it needs a comment somewhere > > about the asymmetric subtlety. > > Is this "somewhere" at every alloc_dax() call site, or do you have > something else in mind ? At least kill_dax() should mention the asymmetry I think. > > > > >> > >> > >>> return; > >>> > >>> if (dax_dev->holder_data != NULL) > >>> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > >>> index 4e8fdcb3f1c8..b69c9e442cf4 100644 > >>> --- a/drivers/nvdimm/pmem.c > >>> +++ b/drivers/nvdimm/pmem.c > >>> @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev, > >>> dax_dev = alloc_dax(pmem, &pmem_dax_ops); > >>> if (IS_ERR(dax_dev)) { > >>> rc = PTR_ERR(dax_dev); > >>> - goto out; > >>> + if (rc != -EOPNOTSUPP) > >>> + goto out; > >> > >> If I compare the before / after this change, if previously > >> pmem_attach_disk() was called in a configuration with FS_DAX=n, it would > >> result in a NULL pointer dereference. > > > > No, alloc_dax() only returns NULL CONFIG_DAX=n case, not the > > CONFIG_FS_DAX=n case. > > Indeed, I was wrong there. > > > So that means that pmem devices on ARM have been > > possible without FS_DAX. So, in order for alloc_dax() returning > > ERR_PTR(-EOPNOTSUPP) to not regress pmem device availability this error > > path needs to be changed. > Good point. We're moving the depends on !(ARM || MIPS |PARC) from FS_DAX > Kconfig to a runtime check in alloc_dax(), which is used whenever DAX=y, > which includes configurations that had FS_DAX=n previously. > > I'll change the error path in pmem_attack_disk to treat -EOPNOTSUPP > alloc_dax() return value as non-fatal. > > > > >> This would be an error handling fix all by itself. Do we really want > >> to return successfully if dax is unsupported, or should we return > >> an error here ? > > > > Per above, there is no error handling fix, and pmem block device > > available should not depend on alloc_dax() succeeding. > > I agree on treating alloc_dax() failure as non-fatal. There is > however one error handling fix to nvdimm/pmem which I plan to > introduce as an initial patch before this change: > > nvdimm/pmem: Fix leak on dax_add_host() failure > > Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done > before setting pmem->dax_dev, which therefore issues the two following > calls on NULL pointers: > > out_cleanup_dax: > kill_dax(pmem->dax_dev); > put_dax(pmem->dax_dev); > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c > index 4e8fdcb3f1c8..9fe358090720 100644 > --- a/drivers/nvdimm/pmem.c > +++ b/drivers/nvdimm/pmem.c > @@ -566,12 +566,11 @@ static int pmem_attach_disk(struct device *dev, > set_dax_nomc(dax_dev); > if (is_nvdimm_sync(nd_region)) > set_dax_synchronous(dax_dev); > + pmem->dax_dev = dax_dev; > rc = dax_add_host(dax_dev, disk); > if (rc) > goto out_cleanup_dax; > dax_write_cache(dax_dev, nvdimm_has_cache(nd_region)); > - pmem->dax_dev = dax_dev; > - > rc = device_add_disk(dev, disk, pmem_attribute_groups); > if (rc) > goto out_remove_host; Yup, looks good. > > The real question is what to do about device-dax. I *think* it is not > > affected by cpu_dcache aliasing because it never accesses user mappings > > through a kernel alias. I doubt device-dax is in use on these platforms, > > but we might need another fixup for that if someone screams about the > > alloc_dax() behavior change making them lose device-dax access. > > By "device-dax", I understand you mean drivers/dax/Kconfig:DEV_DAX. > > Based on your analysis, is alloc_dax() still the right spot where > to place this runtime check ? Which call sites are responsible > for invoking alloc_dax() for device-dax ? That is in devm_create_dev_dax(). > If we know which call sites do not intend to use the kernel linear > mapping, we could introduce a flag (or a new variant of the alloc_dax() > API) that would either enforce or skip the check. Hmmm, it looks like there is already a natural flag for that. If alloc_dax() is passed a NULL operations pointer it means there are no kernel usages of the aliased mapping. That actually fits rather nicely. [..] > >>> @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) > >>> if (!IS_ENABLED(CONFIG_FUSE_DAX)) > >>> return 0; > >>> > >>> + dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); > >>> + if (IS_ERR(dax_dev)) { > >>> + int rc = PTR_ERR(dax_dev); > >>> + > >>> + if (rc == -EOPNOTSUPP) > >>> + return 0; > >>> + return rc; > >>> + } > >> > >> What is gained by moving this allocation here ? > > > > The gain is to fail early in virtio_fs_setup_dax() since the fundamental > > dependency of alloc_dax() success is not met. For example why let the > > setup progress to devm_memremap_pages() when alloc_dax() is going to > > return ERR_PTR(-EOPNOTSUPP). > > What I don't know is whether there is a dependency requiring to do > devm_request_mem_region(), devm_kzalloc(), devm_memremap_pages() > before calling alloc_dax() ? > > Those 3 calls are used to populate: > > fs->window_phys_addr = (phys_addr_t) cache_reg.addr; > fs->window_len = (phys_addr_t) cache_reg.len; > > and then alloc_dax() takes "fs" as private data parameter. So it's > unclear to me whether we can swap the invocation order. I suspect > that it is not an issue because it is only used to populate > dax_dev->private, but I prefer to confirm this with you just to be > on the safe side. Thanks for that. All of those need to be done before the fs goes live later in virtio_device_ready(), but before that point nothing should be calling into virtio_fs_dax_ops, so as far as I can see it is safe to change the order.
On 2024-02-02 14:41, Dan Williams wrote: > Mathieu Desnoyers wrote: >> On 2024-02-02 12:37, Dan Williams wrote: >>> Mathieu Desnoyers wrote: >> [...] >>>> >>> >>>> The alternative route I intend to take is to audit all callers >>>> of alloc_dax() and make sure they all save the alloc_dax() return >>>> value in a struct dax_device * local variable first for the sake >>>> of checking for IS_ERR(). This will leave the xyz->dax_dev pointer >>>> initialized to NULL in the error case and simplify the rest of >>>> error checking. >>> >>> I could maybe get on board with that, but it needs a comment somewhere >>> about the asymmetric subtlety. >> >> Is this "somewhere" at every alloc_dax() call site, or do you have >> something else in mind ? > > At least kill_dax() should mention the asymmetry I think. Here is what I intend to add: * Note, because alloc_dax() returns an ERR_PTR() on error, callers * typically store its result into a local variable in order to check * the result. Therefore, care must be taken to populate the struct * device dax_dev field make sure the dax_dev is not leaked. > >>> The real question is what to do about device-dax. I *think* it is not >>> affected by cpu_dcache aliasing because it never accesses user mappings >>> through a kernel alias. I doubt device-dax is in use on these platforms, >>> but we might need another fixup for that if someone screams about the >>> alloc_dax() behavior change making them lose device-dax access. >> >> By "device-dax", I understand you mean drivers/dax/Kconfig:DEV_DAX. >> >> Based on your analysis, is alloc_dax() still the right spot where >> to place this runtime check ? Which call sites are responsible >> for invoking alloc_dax() for device-dax ? > > That is in devm_create_dev_dax(). > >> If we know which call sites do not intend to use the kernel linear >> mapping, we could introduce a flag (or a new variant of the alloc_dax() >> API) that would either enforce or skip the check. > > Hmmm, it looks like there is already a natural flag for that. If > alloc_dax() is passed a NULL operations pointer it means there are no > kernel usages of the aliased mapping. That actually fits rather nicely. Good, I was reaching the same conclusion when I received your reply. I'll do that. It ends up being: /* * Unavailable on architectures with virtually aliased data caches, * except for device-dax (NULL operations pointer), which does * not use aliased mappings from the kernel. */ if (ops && cpu_dcache_is_aliasing()) return ERR_PTR(-EOPNOTSUPP); > > [..] >>>>> @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs) >>>>> if (!IS_ENABLED(CONFIG_FUSE_DAX)) >>>>> return 0; >>>>> >>>>> + dax_dev = alloc_dax(fs, &virtio_fs_dax_ops); >>>>> + if (IS_ERR(dax_dev)) { >>>>> + int rc = PTR_ERR(dax_dev); >>>>> + >>>>> + if (rc == -EOPNOTSUPP) >>>>> + return 0; >>>>> + return rc; >>>>> + } >>>> >>>> What is gained by moving this allocation here ? >>> >>> The gain is to fail early in virtio_fs_setup_dax() since the fundamental >>> dependency of alloc_dax() success is not met. For example why let the >>> setup progress to devm_memremap_pages() when alloc_dax() is going to >>> return ERR_PTR(-EOPNOTSUPP). >> >> What I don't know is whether there is a dependency requiring to do >> devm_request_mem_region(), devm_kzalloc(), devm_memremap_pages() >> before calling alloc_dax() ? >> >> Those 3 calls are used to populate: >> >> fs->window_phys_addr = (phys_addr_t) cache_reg.addr; >> fs->window_len = (phys_addr_t) cache_reg.len; >> >> and then alloc_dax() takes "fs" as private data parameter. So it's >> unclear to me whether we can swap the invocation order. I suspect >> that it is not an issue because it is only used to populate >> dax_dev->private, but I prefer to confirm this with you just to be >> on the safe side. > > Thanks for that. All of those need to be done before the fs goes live > later in virtio_device_ready(), but before that point nothing should be > calling into virtio_fs_dax_ops, so as far as I can see it is safe to > change the order. Sounds good, I'll do that. I will soon be ready to send out a RFC v4, which is still only compiled-tested. Do you happen to have some kind of test suite you can use to automate some of the runtime testing ? Thanks, Mathieu
Mathieu Desnoyers wrote: [..] > > Thanks for that. All of those need to be done before the fs goes live > > later in virtio_device_ready(), but before that point nothing should be > > calling into virtio_fs_dax_ops, so as far as I can see it is safe to > > change the order. > > Sounds good, I'll do that. > > I will soon be ready to send out a RFC v4, which is still only > compiled-tested. Do you happen to have some kind of test suite > you can use to automate some of the runtime testing ? There is a test suite for the pmem, dm, and dax changes (https://github.com/pmem/ndctl?tab=readme-ov-file#unit-tests), but not automated unfortunately. The NVDIMM maintainer team will run that before pushing patches out to the fixes branch if you just want to lean on that. For the rest I think we will need to depend on tested-by's from s390 + virtio_fs folks, and / or sufficient soak time in linux-next.
On 2024-02-02 15:14, Dan Williams wrote: > Mathieu Desnoyers wrote: > [..] >>> Thanks for that. All of those need to be done before the fs goes live >>> later in virtio_device_ready(), but before that point nothing should be >>> calling into virtio_fs_dax_ops, so as far as I can see it is safe to >>> change the order. >> >> Sounds good, I'll do that. >> >> I will soon be ready to send out a RFC v4, which is still only >> compiled-tested. Do you happen to have some kind of test suite >> you can use to automate some of the runtime testing ? > > There is a test suite for the pmem, dm, and dax changes > (https://github.com/pmem/ndctl?tab=readme-ov-file#unit-tests), but not > automated unfortunately. The NVDIMM maintainer team will run that before > pushing patches out to the fixes branch if you just want to lean on > that. For the rest I think we will need to depend on tested-by's from > s390 + virtio_fs folks, and / or sufficient soak time in linux-next. I suspect this will be necessary. There are just so many combinations of architectures, drivers and filesystems involved here that I don't think it is realistic to try to do all this testing manually on my own. I prefer to voice this up front, so there are no misplaced expectations about testing. Thanks, Mathieu
diff --git a/drivers/dax/super.c b/drivers/dax/super.c index 0da9232ea175..e9f397b8a5a3 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -445,6 +445,12 @@ struct dax_device *alloc_dax(void *private, const struct dax_operations *ops) dev_t devt; int minor; + /* Unavailable on architectures with virtually aliased data caches. */ + if (IS_ENABLED(CONFIG_ARM) || + IS_ENABLED(CONFIG_MIPS) || + IS_ENABLED(CONFIG_SPARC)) + return NULL; + if (WARN_ON_ONCE(ops && !ops->zero_page_range)) return ERR_PTR(-EINVAL); diff --git a/fs/Kconfig b/fs/Kconfig index 42837617a55b..e5efdb3b276b 100644 --- a/fs/Kconfig +++ b/fs/Kconfig @@ -56,7 +56,6 @@ endif # BLOCK config FS_DAX bool "File system based Direct Access (DAX) support" depends on MMU - depends on !(ARM || MIPS || SPARC) depends on ZONE_DEVICE || FS_DAX_LIMITED select FS_IOMAP select DAX
Replace the following fs/Kconfig:FS_DAX dependency: depends on !(ARM || MIPS || SPARC) By a runtime check within alloc_dax(). This is done in preparation for its use by each filesystem supporting the "dax" mount option to validate whether DAX is indeed supported. This is done in preparation for using cpu_dcache_is_aliasing() in a following change which will properly support architectures which detect data cache aliasing at runtime. Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches") Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-mm@kvack.org Cc: linux-arch@vger.kernel.org Cc: Dan Williams <dan.j.williams@intel.com> Cc: Vishal Verma <vishal.l.verma@intel.com> Cc: Dave Jiang <dave.jiang@intel.com> Cc: Matthew Wilcox <willy@infradead.org> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Russell King <linux@armlinux.org.uk> Cc: nvdimm@lists.linux.dev Cc: linux-cxl@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: dm-devel@lists.linux.dev --- drivers/dax/super.c | 6 ++++++ fs/Kconfig | 1 - 2 files changed, 6 insertions(+), 1 deletion(-)