| Message ID | 168577285161.1672036.8111253437794419696.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2532f41607c4308733239dd43278f8a5540f3ec7 |
| Headers | show |
| Series | dax: Fix use after free and other cleanups | expand |
Dan Williams wrote: > Now that free_dev_dax_id() internally manages the references it needs > the extra references taken by the dax_region drivers are not needed. > > Reported-by: Ira Weiny <ira.weiny@intel.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/dax/bus.c | 4 +--- > drivers/dax/bus.h | 1 - > drivers/dax/cxl.c | 8 +------- > drivers/dax/hmem/hmem.c | 8 +------- > drivers/dax/pmem.c | 7 +------ > 5 files changed, 4 insertions(+), 24 deletions(-) > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index a4cc3eca774f..0ee96e6fc426 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -454,11 +454,10 @@ static void dax_region_free(struct kref *kref) > kfree(dax_region); > } > > -void dax_region_put(struct dax_region *dax_region) > +static void dax_region_put(struct dax_region *dax_region) > { > kref_put(&dax_region->kref, dax_region_free); > } > -EXPORT_SYMBOL_GPL(dax_region_put); > > /* a return value >= 0 indicates this invocation invalidated the id */ > static int __free_dev_dax_id(struct dev_dax *dev_dax) > @@ -641,7 +640,6 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id, > return NULL; > } > > - kref_get(&dax_region->kref); > if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region)) > return NULL; > return dax_region; > diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h > index 8cd79ab34292..bdbf719df5c5 100644 > --- a/drivers/dax/bus.h > +++ b/drivers/dax/bus.h > @@ -9,7 +9,6 @@ struct dev_dax; > struct resource; > struct dax_device; > struct dax_region; > -void dax_region_put(struct dax_region *dax_region); > > /* dax bus specific ioresource flags */ > #define IORESOURCE_DAX_STATIC BIT(0) > diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c > index ccdf8de85bd5..8bc9d04034d6 100644 > --- a/drivers/dax/cxl.c > +++ b/drivers/dax/cxl.c > @@ -13,7 +13,6 @@ static int cxl_dax_region_probe(struct device *dev) > struct cxl_region *cxlr = cxlr_dax->cxlr; > struct dax_region *dax_region; > struct dev_dax_data data; > - struct dev_dax *dev_dax; > > if (nid == NUMA_NO_NODE) > nid = memory_add_physaddr_to_nid(cxlr_dax->hpa_range.start); > @@ -28,13 +27,8 @@ static int cxl_dax_region_probe(struct device *dev) > .id = -1, > .size = range_len(&cxlr_dax->hpa_range), > }; > - dev_dax = devm_create_dev_dax(&data); > - if (IS_ERR(dev_dax)) > - return PTR_ERR(dev_dax); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - return 0; > + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); > } > > static struct cxl_driver cxl_dax_region_driver = { > diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c > index e5fe8b39fb94..5d2ddef0f8f5 100644 > --- a/drivers/dax/hmem/hmem.c > +++ b/drivers/dax/hmem/hmem.c > @@ -16,7 +16,6 @@ static int dax_hmem_probe(struct platform_device *pdev) > struct dax_region *dax_region; > struct memregion_info *mri; > struct dev_dax_data data; > - struct dev_dax *dev_dax; > > /* > * @region_idle == true indicates that an administrative agent > @@ -38,13 +37,8 @@ static int dax_hmem_probe(struct platform_device *pdev) > .id = -1, > .size = region_idle ? 0 : range_len(&mri->range), > }; > - dev_dax = devm_create_dev_dax(&data); > - if (IS_ERR(dev_dax)) > - return PTR_ERR(dev_dax); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - return 0; > + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); > } > > static struct platform_driver dax_hmem_driver = { > diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c > index f050ea78bb83..ae0cb113a5d3 100644 > --- a/drivers/dax/pmem.c > +++ b/drivers/dax/pmem.c > @@ -13,7 +13,6 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > int rc, id, region_id; > resource_size_t offset; > struct nd_pfn_sb *pfn_sb; > - struct dev_dax *dev_dax; > struct dev_dax_data data; > struct nd_namespace_io *nsio; > struct dax_region *dax_region; > @@ -65,12 +64,8 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > .pgmap = &pgmap, > .size = range_len(&range), > }; > - dev_dax = devm_create_dev_dax(&data); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - > - return dev_dax; > + return devm_create_dev_dax(&data); > } > > static int dax_pmem_probe(struct device *dev) >
The 06/02/2023 23:14, Dan Williams wrote: > Now that free_dev_dax_id() internally manages the references it needs > the extra references taken by the dax_region drivers are not needed. > > Reported-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Fan Ni <fan.ni@samsung.com> One minor comment as below. > --- > drivers/dax/bus.c | 4 +--- > drivers/dax/bus.h | 1 - > drivers/dax/cxl.c | 8 +------- > drivers/dax/hmem/hmem.c | 8 +------- > drivers/dax/pmem.c | 7 +------ > 5 files changed, 4 insertions(+), 24 deletions(-) > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index a4cc3eca774f..0ee96e6fc426 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -454,11 +454,10 @@ static void dax_region_free(struct kref *kref) > kfree(dax_region); > } > > -void dax_region_put(struct dax_region *dax_region) > +static void dax_region_put(struct dax_region *dax_region) > { > kref_put(&dax_region->kref, dax_region_free); > } > -EXPORT_SYMBOL_GPL(dax_region_put); > > /* a return value >= 0 indicates this invocation invalidated the id */ > static int __free_dev_dax_id(struct dev_dax *dev_dax) > @@ -641,7 +640,6 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id, > return NULL; > } > > - kref_get(&dax_region->kref); > if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region)) > return NULL; > return dax_region; > diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h > index 8cd79ab34292..bdbf719df5c5 100644 > --- a/drivers/dax/bus.h > +++ b/drivers/dax/bus.h > @@ -9,7 +9,6 @@ struct dev_dax; > struct resource; > struct dax_device; > struct dax_region; > -void dax_region_put(struct dax_region *dax_region); > > /* dax bus specific ioresource flags */ > #define IORESOURCE_DAX_STATIC BIT(0) > diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c > index ccdf8de85bd5..8bc9d04034d6 100644 > --- a/drivers/dax/cxl.c > +++ b/drivers/dax/cxl.c > @@ -13,7 +13,6 @@ static int cxl_dax_region_probe(struct device *dev) > struct cxl_region *cxlr = cxlr_dax->cxlr; > struct dax_region *dax_region; > struct dev_dax_data data; > - struct dev_dax *dev_dax; > > if (nid == NUMA_NO_NODE) > nid = memory_add_physaddr_to_nid(cxlr_dax->hpa_range.start); > @@ -28,13 +27,8 @@ static int cxl_dax_region_probe(struct device *dev) > .id = -1, > .size = range_len(&cxlr_dax->hpa_range), > }; > - dev_dax = devm_create_dev_dax(&data); > - if (IS_ERR(dev_dax)) > - return PTR_ERR(dev_dax); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - return 0; > + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); > } > > static struct cxl_driver cxl_dax_region_driver = { > diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c > index e5fe8b39fb94..5d2ddef0f8f5 100644 > --- a/drivers/dax/hmem/hmem.c > +++ b/drivers/dax/hmem/hmem.c > @@ -16,7 +16,6 @@ static int dax_hmem_probe(struct platform_device *pdev) > struct dax_region *dax_region; > struct memregion_info *mri; > struct dev_dax_data data; > - struct dev_dax *dev_dax; > > /* > * @region_idle == true indicates that an administrative agent > @@ -38,13 +37,8 @@ static int dax_hmem_probe(struct platform_device *pdev) > .id = -1, > .size = region_idle ? 0 : range_len(&mri->range), > }; > - dev_dax = devm_create_dev_dax(&data); > - if (IS_ERR(dev_dax)) > - return PTR_ERR(dev_dax); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - return 0; > + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); > } > > static struct platform_driver dax_hmem_driver = { > diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c > index f050ea78bb83..ae0cb113a5d3 100644 > --- a/drivers/dax/pmem.c > +++ b/drivers/dax/pmem.c > @@ -13,7 +13,6 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > int rc, id, region_id; > resource_size_t offset; > struct nd_pfn_sb *pfn_sb; > - struct dev_dax *dev_dax; > struct dev_dax_data data; > struct nd_namespace_io *nsio; > struct dax_region *dax_region; > @@ -65,12 +64,8 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > .pgmap = &pgmap, > .size = range_len(&range), > }; > - dev_dax = devm_create_dev_dax(&data); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - > - return dev_dax; > + return devm_create_dev_dax(&data); Not related to the patch, but why we do not need to check the returned value of devm_create_dev_dax as above? Or do we really need the check as the function already returns ERR_PTR if failed? Fan > } > > static int dax_pmem_probe(struct device *dev) >
Fan Ni wrote: > The 06/02/2023 23:14, Dan Williams wrote: > > Now that free_dev_dax_id() internally manages the references it needs > > the extra references taken by the dax_region drivers are not needed. > > > > Reported-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > Reviewed-by: Fan Ni <fan.ni@samsung.com> > One minor comment as below. > [snip] > > static struct platform_driver dax_hmem_driver = { > > diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c > > index f050ea78bb83..ae0cb113a5d3 100644 > > --- a/drivers/dax/pmem.c > > +++ b/drivers/dax/pmem.c > > @@ -13,7 +13,6 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > > int rc, id, region_id; > > resource_size_t offset; > > struct nd_pfn_sb *pfn_sb; > > - struct dev_dax *dev_dax; > > struct dev_dax_data data; > > struct nd_namespace_io *nsio; > > struct dax_region *dax_region; > > @@ -65,12 +64,8 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > > .pgmap = &pgmap, > > .size = range_len(&range), > > }; > > - dev_dax = devm_create_dev_dax(&data); > > > > - /* child dev_dax instances now own the lifetime of the dax_region */ > > - dax_region_put(dax_region); > > - > > - return dev_dax; > > + return devm_create_dev_dax(&data); > > Not related to the patch, but why we do not need to check the returned > value of devm_create_dev_dax as above? __dax_pmem_probe() returns struct dev_dax * so we just pass the result on. > Or do we really need the check as > the function already returns ERR_PTR if failed? Yea the caller of __dax_pmem_probe() needs to handle it. Ira
On 6/2/23 23:14, Dan Williams wrote: > Now that free_dev_dax_id() internally manages the references it needs > the extra references taken by the dax_region drivers are not needed. > > Reported-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/dax/bus.c | 4 +--- > drivers/dax/bus.h | 1 - > drivers/dax/cxl.c | 8 +------- > drivers/dax/hmem/hmem.c | 8 +------- > drivers/dax/pmem.c | 7 +------ > 5 files changed, 4 insertions(+), 24 deletions(-) > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index a4cc3eca774f..0ee96e6fc426 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -454,11 +454,10 @@ static void dax_region_free(struct kref *kref) > kfree(dax_region); > } > > -void dax_region_put(struct dax_region *dax_region) > +static void dax_region_put(struct dax_region *dax_region) > { > kref_put(&dax_region->kref, dax_region_free); > } > -EXPORT_SYMBOL_GPL(dax_region_put); > > /* a return value >= 0 indicates this invocation invalidated the id */ > static int __free_dev_dax_id(struct dev_dax *dev_dax) > @@ -641,7 +640,6 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id, > return NULL; > } > > - kref_get(&dax_region->kref); > if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region)) > return NULL; > return dax_region; > diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h > index 8cd79ab34292..bdbf719df5c5 100644 > --- a/drivers/dax/bus.h > +++ b/drivers/dax/bus.h > @@ -9,7 +9,6 @@ struct dev_dax; > struct resource; > struct dax_device; > struct dax_region; > -void dax_region_put(struct dax_region *dax_region); > > /* dax bus specific ioresource flags */ > #define IORESOURCE_DAX_STATIC BIT(0) > diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c > index ccdf8de85bd5..8bc9d04034d6 100644 > --- a/drivers/dax/cxl.c > +++ b/drivers/dax/cxl.c > @@ -13,7 +13,6 @@ static int cxl_dax_region_probe(struct device *dev) > struct cxl_region *cxlr = cxlr_dax->cxlr; > struct dax_region *dax_region; > struct dev_dax_data data; > - struct dev_dax *dev_dax; > > if (nid == NUMA_NO_NODE) > nid = memory_add_physaddr_to_nid(cxlr_dax->hpa_range.start); > @@ -28,13 +27,8 @@ static int cxl_dax_region_probe(struct device *dev) > .id = -1, > .size = range_len(&cxlr_dax->hpa_range), > }; > - dev_dax = devm_create_dev_dax(&data); > - if (IS_ERR(dev_dax)) > - return PTR_ERR(dev_dax); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - return 0; > + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); > } > > static struct cxl_driver cxl_dax_region_driver = { > diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c > index e5fe8b39fb94..5d2ddef0f8f5 100644 > --- a/drivers/dax/hmem/hmem.c > +++ b/drivers/dax/hmem/hmem.c > @@ -16,7 +16,6 @@ static int dax_hmem_probe(struct platform_device *pdev) > struct dax_region *dax_region; > struct memregion_info *mri; > struct dev_dax_data data; > - struct dev_dax *dev_dax; > > /* > * @region_idle == true indicates that an administrative agent > @@ -38,13 +37,8 @@ static int dax_hmem_probe(struct platform_device *pdev) > .id = -1, > .size = region_idle ? 0 : range_len(&mri->range), > }; > - dev_dax = devm_create_dev_dax(&data); > - if (IS_ERR(dev_dax)) > - return PTR_ERR(dev_dax); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - return 0; > + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); > } > > static struct platform_driver dax_hmem_driver = { > diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c > index f050ea78bb83..ae0cb113a5d3 100644 > --- a/drivers/dax/pmem.c > +++ b/drivers/dax/pmem.c > @@ -13,7 +13,6 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > int rc, id, region_id; > resource_size_t offset; > struct nd_pfn_sb *pfn_sb; > - struct dev_dax *dev_dax; > struct dev_dax_data data; > struct nd_namespace_io *nsio; > struct dax_region *dax_region; > @@ -65,12 +64,8 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) > .pgmap = &pgmap, > .size = range_len(&range), > }; > - dev_dax = devm_create_dev_dax(&data); > > - /* child dev_dax instances now own the lifetime of the dax_region */ > - dax_region_put(dax_region); > - > - return dev_dax; > + return devm_create_dev_dax(&data); > } > > static int dax_pmem_probe(struct device *dev) > >
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index a4cc3eca774f..0ee96e6fc426 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -454,11 +454,10 @@ static void dax_region_free(struct kref *kref) kfree(dax_region); } -void dax_region_put(struct dax_region *dax_region) +static void dax_region_put(struct dax_region *dax_region) { kref_put(&dax_region->kref, dax_region_free); } -EXPORT_SYMBOL_GPL(dax_region_put); /* a return value >= 0 indicates this invocation invalidated the id */ static int __free_dev_dax_id(struct dev_dax *dev_dax) @@ -641,7 +640,6 @@ struct dax_region *alloc_dax_region(struct device *parent, int region_id, return NULL; } - kref_get(&dax_region->kref); if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region)) return NULL; return dax_region; diff --git a/drivers/dax/bus.h b/drivers/dax/bus.h index 8cd79ab34292..bdbf719df5c5 100644 --- a/drivers/dax/bus.h +++ b/drivers/dax/bus.h @@ -9,7 +9,6 @@ struct dev_dax; struct resource; struct dax_device; struct dax_region; -void dax_region_put(struct dax_region *dax_region); /* dax bus specific ioresource flags */ #define IORESOURCE_DAX_STATIC BIT(0) diff --git a/drivers/dax/cxl.c b/drivers/dax/cxl.c index ccdf8de85bd5..8bc9d04034d6 100644 --- a/drivers/dax/cxl.c +++ b/drivers/dax/cxl.c @@ -13,7 +13,6 @@ static int cxl_dax_region_probe(struct device *dev) struct cxl_region *cxlr = cxlr_dax->cxlr; struct dax_region *dax_region; struct dev_dax_data data; - struct dev_dax *dev_dax; if (nid == NUMA_NO_NODE) nid = memory_add_physaddr_to_nid(cxlr_dax->hpa_range.start); @@ -28,13 +27,8 @@ static int cxl_dax_region_probe(struct device *dev) .id = -1, .size = range_len(&cxlr_dax->hpa_range), }; - dev_dax = devm_create_dev_dax(&data); - if (IS_ERR(dev_dax)) - return PTR_ERR(dev_dax); - /* child dev_dax instances now own the lifetime of the dax_region */ - dax_region_put(dax_region); - return 0; + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); } static struct cxl_driver cxl_dax_region_driver = { diff --git a/drivers/dax/hmem/hmem.c b/drivers/dax/hmem/hmem.c index e5fe8b39fb94..5d2ddef0f8f5 100644 --- a/drivers/dax/hmem/hmem.c +++ b/drivers/dax/hmem/hmem.c @@ -16,7 +16,6 @@ static int dax_hmem_probe(struct platform_device *pdev) struct dax_region *dax_region; struct memregion_info *mri; struct dev_dax_data data; - struct dev_dax *dev_dax; /* * @region_idle == true indicates that an administrative agent @@ -38,13 +37,8 @@ static int dax_hmem_probe(struct platform_device *pdev) .id = -1, .size = region_idle ? 0 : range_len(&mri->range), }; - dev_dax = devm_create_dev_dax(&data); - if (IS_ERR(dev_dax)) - return PTR_ERR(dev_dax); - /* child dev_dax instances now own the lifetime of the dax_region */ - dax_region_put(dax_region); - return 0; + return PTR_ERR_OR_ZERO(devm_create_dev_dax(&data)); } static struct platform_driver dax_hmem_driver = { diff --git a/drivers/dax/pmem.c b/drivers/dax/pmem.c index f050ea78bb83..ae0cb113a5d3 100644 --- a/drivers/dax/pmem.c +++ b/drivers/dax/pmem.c @@ -13,7 +13,6 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) int rc, id, region_id; resource_size_t offset; struct nd_pfn_sb *pfn_sb; - struct dev_dax *dev_dax; struct dev_dax_data data; struct nd_namespace_io *nsio; struct dax_region *dax_region; @@ -65,12 +64,8 @@ static struct dev_dax *__dax_pmem_probe(struct device *dev) .pgmap = &pgmap, .size = range_len(&range), }; - dev_dax = devm_create_dev_dax(&data); - /* child dev_dax instances now own the lifetime of the dax_region */ - dax_region_put(dax_region); - - return dev_dax; + return devm_create_dev_dax(&data); } static int dax_pmem_probe(struct device *dev)
Now that free_dev_dax_id() internally manages the references it needs the extra references taken by the dax_region drivers are not needed. Reported-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/dax/bus.c | 4 +--- drivers/dax/bus.h | 1 - drivers/dax/cxl.c | 8 +------- drivers/dax/hmem/hmem.c | 8 +------- drivers/dax/pmem.c | 7 +------ 5 files changed, 4 insertions(+), 24 deletions(-)