| Message ID | 168577284563.1672036.13493034988900989554.stgit@dwillia2-xfh.jf.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 70aab281e18c68a1284bc387de127c2fc0bed3f8 |
| Headers | show |
| Series | dax: Fix use after free and other cleanups | expand |
Dan Williams wrote: > The reference counting of dax_region objects is needlessly complicated, > has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up > that mess introduce alloc_dev_dax_id() to minimize the holding of a > dax_region reference to only what dev_dax_release() needs, the > dax_region->ida. > > Part of the reason for the mess was the design to dereference a > dax_region in all cases in free_dev_dax_id() even if the id was > statically assigned by the upper level dax_region driver. Remove the > need to call "is_static(dax_region)" by tracking whether the id is > dynamic directly in the dev_dax instance itself. > > With that flag the dax_region pinning and release per dev_dax instance > can move to alloc_dev_dax_id() and free_dev_dax_id() respectively. > > A follow-on cleanup address the unnecessary references in the dax_region > setup and drivers. > > Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices") > Link: http://lore.kernel.org/r/20221203095858.612027-1-liuyongqiang13@huawei.com [1] > Link: http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com [2] > Reported-by: Yongqiang Liu <liuyongqiang13@huawei.com> > Reported-by: Paul Cassella <cassella@hpe.com> > Reported-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/dax/bus.c | 56 +++++++++++++++++++++++++++------------------ > drivers/dax/dax-private.h | 4 ++- > 2 files changed, 37 insertions(+), 23 deletions(-) > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index c99ea08aafc3..a4cc3eca774f 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -446,18 +446,34 @@ static void unregister_dev_dax(void *dev) > put_device(dev); > } > > +static void dax_region_free(struct kref *kref) > +{ > + struct dax_region *dax_region; > + > + dax_region = container_of(kref, struct dax_region, kref); > + kfree(dax_region); > +} > + > +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) > { > - struct dax_region *dax_region = dev_dax->region; > struct device *dev = &dev_dax->dev; > + struct dax_region *dax_region; > int rc = dev_dax->id; > > device_lock_assert(dev); > > - if (is_static(dax_region) || dev_dax->id < 0) > + if (!dev_dax->dyn_id || dev_dax->id < 0) > return -1; > + dax_region = dev_dax->region; > ida_free(&dax_region->ida, dev_dax->id); > + dax_region_put(dax_region); > dev_dax->id = -1; > return rc; > } > @@ -473,6 +489,20 @@ static int free_dev_dax_id(struct dev_dax *dev_dax) > return rc; > } > > +static int alloc_dev_dax_id(struct dev_dax *dev_dax) > +{ > + struct dax_region *dax_region = dev_dax->region; > + int id; > + > + id = ida_alloc(&dax_region->ida, GFP_KERNEL); > + if (id < 0) > + return id; > + kref_get(&dax_region->kref); > + dev_dax->dyn_id = true; > + dev_dax->id = id; > + return id; > +} > + > static ssize_t delete_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t len) > { > @@ -560,20 +590,6 @@ static const struct attribute_group *dax_region_attribute_groups[] = { > NULL, > }; > > -static void dax_region_free(struct kref *kref) > -{ > - struct dax_region *dax_region; > - > - dax_region = container_of(kref, struct dax_region, kref); > - kfree(dax_region); > -} > - > -void dax_region_put(struct dax_region *dax_region) > -{ > - kref_put(&dax_region->kref, dax_region_free); > -} > -EXPORT_SYMBOL_GPL(dax_region_put); > - > static void dax_region_unregister(void *region) > { > struct dax_region *dax_region = region; > @@ -1297,12 +1313,10 @@ static const struct attribute_group *dax_attribute_groups[] = { > static void dev_dax_release(struct device *dev) > { > struct dev_dax *dev_dax = to_dev_dax(dev); > - struct dax_region *dax_region = dev_dax->region; > struct dax_device *dax_dev = dev_dax->dax_dev; > > put_dax(dax_dev); > free_dev_dax_id(dev_dax); > - dax_region_put(dax_region); > kfree(dev_dax->pgmap); > kfree(dev_dax); > } > @@ -1326,6 +1340,7 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) > if (!dev_dax) > return ERR_PTR(-ENOMEM); > > + dev_dax->region = dax_region; Overall I like that this reference is not needed to be carried and/or managed by the callers. However, here you are referencing the dax_region from the dev_dax in an unrelated place to where the reference matters (in id management). Could alloc_dev_dax_id() change to: static int alloc_dev_dax_id(struct dev_dax *dev_dax, struct dax_region *dax_region) { ... } Then make this assignment next to where the kref is taken so it is clear that this is the only user of the reference? I did not pick up on the fact this reference was only needed to free the id at all in reviewing the code and I think this would make it even more clear. Ira > if (is_static(dax_region)) { > if (dev_WARN_ONCE(parent, data->id < 0, > "dynamic id specified to static region\n")) { > @@ -1341,13 +1356,11 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) > goto err_id; > } > > - rc = ida_alloc(&dax_region->ida, GFP_KERNEL); > + rc = alloc_dev_dax_id(dev_dax); > if (rc < 0) > goto err_id; > - dev_dax->id = rc; > } > > - dev_dax->region = dax_region; > dev = &dev_dax->dev; > device_initialize(dev); > dev_set_name(dev, "dax%d.%d", dax_region->id, dev_dax->id); > @@ -1388,7 +1401,6 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) > dev_dax->target_node = dax_region->target_node; > dev_dax->align = dax_region->align; > ida_init(&dev_dax->ida); > - kref_get(&dax_region->kref); > > inode = dax_inode(dax_dev); > dev->devt = inode->i_rdev; > diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h > index 1c974b7caae6..afcada6fd2ed 100644 > --- a/drivers/dax/dax-private.h > +++ b/drivers/dax/dax-private.h > @@ -52,7 +52,8 @@ struct dax_mapping { > * @region - parent region > * @dax_dev - core dax functionality > * @target_node: effective numa node if dev_dax memory range is onlined > - * @id: ida allocated id > + * @dyn_id: is this a dynamic or statically created instance > + * @id: ida allocated id when the dax_region is not static > * @ida: mapping id allocator > * @dev - device core > * @pgmap - pgmap for memmap setup / lifetime (driver owned) > @@ -64,6 +65,7 @@ struct dev_dax { > struct dax_device *dax_dev; > unsigned int align; > int target_node; > + bool dyn_id; > int id; > struct ida ida; > struct device dev; >
Ira Weiny wrote: > Dan Williams wrote: > > The reference counting of dax_region objects is needlessly complicated, > > has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up > > that mess introduce alloc_dev_dax_id() to minimize the holding of a > > dax_region reference to only what dev_dax_release() needs, the > > dax_region->ida. > > > > Part of the reason for the mess was the design to dereference a > > dax_region in all cases in free_dev_dax_id() even if the id was > > statically assigned by the upper level dax_region driver. Remove the > > need to call "is_static(dax_region)" by tracking whether the id is > > dynamic directly in the dev_dax instance itself. > > > > With that flag the dax_region pinning and release per dev_dax instance > > can move to alloc_dev_dax_id() and free_dev_dax_id() respectively. > > > > A follow-on cleanup address the unnecessary references in the dax_region > > setup and drivers. > > > > Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices") > > Link: http://lore.kernel.org/r/20221203095858.612027-1-liuyongqiang13@huawei.com [1] > > Link: http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com [2] > > Reported-by: Yongqiang Liu <liuyongqiang13@huawei.com> > > Reported-by: Paul Cassella <cassella@hpe.com> > > Reported-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> [..] > > @@ -1326,6 +1340,7 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) > > if (!dev_dax) > > return ERR_PTR(-ENOMEM); > > > > + dev_dax->region = dax_region; > > Overall I like that this reference is not needed to be carried and/or > managed by the callers. > > However, here you are referencing the dax_region from the dev_dax in an > unrelated place to where the reference matters (in id management). > > Could alloc_dev_dax_id() change to: > > static int alloc_dev_dax_id(struct dev_dax *dev_dax, struct dax_region *dax_region) > { > ... > } > > Then make this assignment next to where the kref is taken so it is clear > that this is the only user of the reference? > > I did not pick up on the fact this reference was only needed to free the > id at all in reviewing the code and I think this would make it even more > clear. I hesitate only for symmetry reasons. I.e. that there are many interfaces in this file, in addition to free_dev_dax_id(), where @dax_region is implicitly retrieved from the @dev_dax.
Dan Williams wrote: > Ira Weiny wrote: > > Dan Williams wrote: > > > The reference counting of dax_region objects is needlessly complicated, > > > has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up > > > that mess introduce alloc_dev_dax_id() to minimize the holding of a > > > dax_region reference to only what dev_dax_release() needs, the > > > dax_region->ida. > > > > > > Part of the reason for the mess was the design to dereference a > > > dax_region in all cases in free_dev_dax_id() even if the id was > > > statically assigned by the upper level dax_region driver. Remove the > > > need to call "is_static(dax_region)" by tracking whether the id is > > > dynamic directly in the dev_dax instance itself. > > > > > > With that flag the dax_region pinning and release per dev_dax instance > > > can move to alloc_dev_dax_id() and free_dev_dax_id() respectively. > > > > > > A follow-on cleanup address the unnecessary references in the dax_region > > > setup and drivers. > > > > > > Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices") > > > Link: http://lore.kernel.org/r/20221203095858.612027-1-liuyongqiang13@huawei.com [1] > > > Link: http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com [2] > > > Reported-by: Yongqiang Liu <liuyongqiang13@huawei.com> > > > Reported-by: Paul Cassella <cassella@hpe.com> > > > Reported-by: Ira Weiny <ira.weiny@intel.com> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > [..] > > > @@ -1326,6 +1340,7 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) > > > if (!dev_dax) > > > return ERR_PTR(-ENOMEM); > > > > > > + dev_dax->region = dax_region; > > > > Overall I like that this reference is not needed to be carried and/or > > managed by the callers. > > > > However, here you are referencing the dax_region from the dev_dax in an > > unrelated place to where the reference matters (in id management). > > > > Could alloc_dev_dax_id() change to: > > > > static int alloc_dev_dax_id(struct dev_dax *dev_dax, struct dax_region *dax_region) > > { > > ... > > } > > > > Then make this assignment next to where the kref is taken so it is clear > > that this is the only user of the reference? > > > > I did not pick up on the fact this reference was only needed to free the > > id at all in reviewing the code and I think this would make it even more > > clear. > > I hesitate only for symmetry reasons. I.e. that there are many interfaces in > this file, in addition to free_dev_dax_id(), where @dax_region is > implicitly retrieved from the @dev_dax. Ok but the reason we need this extra reference and for the dax_region to live this long is because the ida within the dax_region. Otherwise the normal device references would be enough, right? Regardless, I've convinced myself this is ok. Reviewed-by: Ira Weiny <ira.weiny@intel.com> Ira
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index c99ea08aafc3..a4cc3eca774f 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -446,18 +446,34 @@ static void unregister_dev_dax(void *dev) put_device(dev); } +static void dax_region_free(struct kref *kref) +{ + struct dax_region *dax_region; + + dax_region = container_of(kref, struct dax_region, kref); + kfree(dax_region); +} + +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) { - struct dax_region *dax_region = dev_dax->region; struct device *dev = &dev_dax->dev; + struct dax_region *dax_region; int rc = dev_dax->id; device_lock_assert(dev); - if (is_static(dax_region) || dev_dax->id < 0) + if (!dev_dax->dyn_id || dev_dax->id < 0) return -1; + dax_region = dev_dax->region; ida_free(&dax_region->ida, dev_dax->id); + dax_region_put(dax_region); dev_dax->id = -1; return rc; } @@ -473,6 +489,20 @@ static int free_dev_dax_id(struct dev_dax *dev_dax) return rc; } +static int alloc_dev_dax_id(struct dev_dax *dev_dax) +{ + struct dax_region *dax_region = dev_dax->region; + int id; + + id = ida_alloc(&dax_region->ida, GFP_KERNEL); + if (id < 0) + return id; + kref_get(&dax_region->kref); + dev_dax->dyn_id = true; + dev_dax->id = id; + return id; +} + static ssize_t delete_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { @@ -560,20 +590,6 @@ static const struct attribute_group *dax_region_attribute_groups[] = { NULL, }; -static void dax_region_free(struct kref *kref) -{ - struct dax_region *dax_region; - - dax_region = container_of(kref, struct dax_region, kref); - kfree(dax_region); -} - -void dax_region_put(struct dax_region *dax_region) -{ - kref_put(&dax_region->kref, dax_region_free); -} -EXPORT_SYMBOL_GPL(dax_region_put); - static void dax_region_unregister(void *region) { struct dax_region *dax_region = region; @@ -1297,12 +1313,10 @@ static const struct attribute_group *dax_attribute_groups[] = { static void dev_dax_release(struct device *dev) { struct dev_dax *dev_dax = to_dev_dax(dev); - struct dax_region *dax_region = dev_dax->region; struct dax_device *dax_dev = dev_dax->dax_dev; put_dax(dax_dev); free_dev_dax_id(dev_dax); - dax_region_put(dax_region); kfree(dev_dax->pgmap); kfree(dev_dax); } @@ -1326,6 +1340,7 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) if (!dev_dax) return ERR_PTR(-ENOMEM); + dev_dax->region = dax_region; if (is_static(dax_region)) { if (dev_WARN_ONCE(parent, data->id < 0, "dynamic id specified to static region\n")) { @@ -1341,13 +1356,11 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) goto err_id; } - rc = ida_alloc(&dax_region->ida, GFP_KERNEL); + rc = alloc_dev_dax_id(dev_dax); if (rc < 0) goto err_id; - dev_dax->id = rc; } - dev_dax->region = dax_region; dev = &dev_dax->dev; device_initialize(dev); dev_set_name(dev, "dax%d.%d", dax_region->id, dev_dax->id); @@ -1388,7 +1401,6 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data) dev_dax->target_node = dax_region->target_node; dev_dax->align = dax_region->align; ida_init(&dev_dax->ida); - kref_get(&dax_region->kref); inode = dax_inode(dax_dev); dev->devt = inode->i_rdev; diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h index 1c974b7caae6..afcada6fd2ed 100644 --- a/drivers/dax/dax-private.h +++ b/drivers/dax/dax-private.h @@ -52,7 +52,8 @@ struct dax_mapping { * @region - parent region * @dax_dev - core dax functionality * @target_node: effective numa node if dev_dax memory range is onlined - * @id: ida allocated id + * @dyn_id: is this a dynamic or statically created instance + * @id: ida allocated id when the dax_region is not static * @ida: mapping id allocator * @dev - device core * @pgmap - pgmap for memmap setup / lifetime (driver owned) @@ -64,6 +65,7 @@ struct dev_dax { struct dax_device *dax_dev; unsigned int align; int target_node; + bool dyn_id; int id; struct ida ida; struct device dev;
The reference counting of dax_region objects is needlessly complicated, has lead to confusion [1], and has hidden a bug [2]. Towards cleaning up that mess introduce alloc_dev_dax_id() to minimize the holding of a dax_region reference to only what dev_dax_release() needs, the dax_region->ida. Part of the reason for the mess was the design to dereference a dax_region in all cases in free_dev_dax_id() even if the id was statically assigned by the upper level dax_region driver. Remove the need to call "is_static(dax_region)" by tracking whether the id is dynamic directly in the dev_dax instance itself. With that flag the dax_region pinning and release per dev_dax instance can move to alloc_dev_dax_id() and free_dev_dax_id() respectively. A follow-on cleanup address the unnecessary references in the dax_region setup and drivers. Fixes: 0f3da14a4f05 ("device-dax: introduce 'seed' devices") Link: http://lore.kernel.org/r/20221203095858.612027-1-liuyongqiang13@huawei.com [1] Link: http://lore.kernel.org/r/3cf0890b-4eb0-e70e-cd9c-2ecc3d496263@hpe.com [2] Reported-by: Yongqiang Liu <liuyongqiang13@huawei.com> Reported-by: Paul Cassella <cassella@hpe.com> Reported-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/dax/bus.c | 56 +++++++++++++++++++++++++++------------------ drivers/dax/dax-private.h | 4 ++- 2 files changed, 37 insertions(+), 23 deletions(-)