* [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 3:28 ` Dan Williams 0 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 3:28 UTC (permalink / raw) To: linux-nvdimm; +Cc: Xiao Guangrong, stable, linux-acpi acpi_evaluate_object() allocates memory. Free the buffer allocated during acpi_nfit_add(). Cc: <stable@vger.kernel.org> Cc: Vishal Verma <vishal.l.verma@intel.com> Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/acpi/nfit.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 0497175ee6cb..008dbaaa2b75 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; sz = obj->buffer.length; + rc = acpi_nfit_init(acpi_desc, sz); } else dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", __func__, (int) obj->type); - } + kfree(buf.pointer); + acpi_desc->nfit = NULL; + } else + rc = acpi_nfit_init(acpi_desc, sz); - rc = acpi_nfit_init(acpi_desc, sz); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); return rc; _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 3:28 ` Dan Williams 0 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 3:28 UTC (permalink / raw) To: linux-nvdimm Cc: Vishal Verma, linux-acpi, stable, Xiao Guangrong, Haozhong Zhang acpi_evaluate_object() allocates memory. Free the buffer allocated during acpi_nfit_add(). Cc: <stable@vger.kernel.org> Cc: Vishal Verma <vishal.l.verma@intel.com> Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/acpi/nfit.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 0497175ee6cb..008dbaaa2b75 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; sz = obj->buffer.length; + rc = acpi_nfit_init(acpi_desc, sz); } else dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", __func__, (int) obj->type); - } + kfree(buf.pointer); + acpi_desc->nfit = NULL; + } else + rc = acpi_nfit_init(acpi_desc, sz); - rc = acpi_nfit_init(acpi_desc, sz); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); return rc; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/2] nfit: cleanup acpi_nfit_init calling convention 2016-07-15 3:28 ` Dan Williams @ 2016-07-15 3:29 ` Dan Williams -1 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 3:29 UTC (permalink / raw) To: linux-nvdimm; +Cc: linux-acpi Pass the nfit buffer as a parameter rather than hanging it off of acpi_desc. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/acpi/nfit.c | 56 +++++++++++++------------------------- drivers/acpi/nfit.h | 3 +- tools/testing/nvdimm/test/nfit.c | 7 +++-- 3 files changed, 24 insertions(+), 42 deletions(-) diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 008dbaaa2b75..a04f2486e1ca 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -2224,12 +2224,11 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc, return 0; } -int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size sz) { struct device *dev = acpi_desc->dev; struct nfit_table_prev prev; const void *end; - u8 *data; int rc; mutex_lock(&acpi_desc->init_mutex); @@ -2254,7 +2253,6 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) list_cut_position(&prev.flushes, &acpi_desc->flushes, acpi_desc->flushes.prev); - data = (u8 *) acpi_desc->nfit; end = data + sz; while (!IS_ERR_OR_NULL(data)) data = add_table(acpi_desc, &prev, data, end); @@ -2377,7 +2375,7 @@ static int acpi_nfit_add(struct acpi_device *adev) struct acpi_table_header *tbl; acpi_status status = AE_OK; acpi_size sz; - int rc; + int rc = 0; status = acpi_get_table_with_size(ACPI_SIG_NFIT, 0, &tbl, &sz); if (ACPI_FAILURE(status)) { @@ -2394,40 +2392,30 @@ static int acpi_nfit_add(struct acpi_device *adev) if (!acpi_desc->nvdimm_bus) return -ENOMEM; - /* - * Save the acpi header for later and then skip it, - * making nfit point to the first nfit table header. - */ + /* Save the acpi header for exporting the revision via sysfs */ acpi_desc->acpi_header = *tbl; - acpi_desc->nfit = (void *) tbl + sizeof(struct acpi_table_nfit); - sz -= sizeof(struct acpi_table_nfit); /* Evaluate _FIT and override with that if present */ status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); if (ACPI_SUCCESS(status) && buf.length > 0) { - union acpi_object *obj; - /* - * Adjust for the acpi_object header of the _FIT - */ - obj = buf.pointer; - if (obj->type == ACPI_TYPE_BUFFER) { - acpi_desc->nfit = - (struct acpi_nfit_header *)obj->buffer.pointer; - sz = obj->buffer.length; - rc = acpi_nfit_init(acpi_desc, sz); - } else + union acpi_object *obj = buf.pointer; + + if (obj->type == ACPI_TYPE_BUFFER) + rc = acpi_nfit_init(acpi_desc, obj->buffer.pointer, + obj->buffer.length); + else dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", __func__, (int) obj->type); kfree(buf.pointer); - acpi_desc->nfit = NULL; } else - rc = acpi_nfit_init(acpi_desc, sz); + /* skip over the lead-in header table */ + rc = acpi_nfit_init(acpi_desc, (void *) tbl + + sizeof(struct acpi_table_nfit), + sz - sizeof(struct acpi_table_nfit)); - if (rc) { + if (rc) nvdimm_bus_unregister(acpi_desc->nvdimm_bus); - return rc; - } - return 0; + return rc; } static int acpi_nfit_remove(struct acpi_device *adev) @@ -2444,9 +2432,8 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) { struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev); struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; - struct acpi_nfit_header *nfit_saved; - union acpi_object *obj; struct device *dev = &adev->dev; + union acpi_object *obj; acpi_status status; int ret; @@ -2482,17 +2469,12 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) goto out_unlock; } - nfit_saved = acpi_desc->nfit; obj = buf.pointer; if (obj->type == ACPI_TYPE_BUFFER) { - acpi_desc->nfit = - (struct acpi_nfit_header *)obj->buffer.pointer; - ret = acpi_nfit_init(acpi_desc, obj->buffer.length); - if (ret) { - /* Merge failed, restore old nfit, and exit */ - acpi_desc->nfit = nfit_saved; + ret = acpi_nfit_init(acpi_desc, obj->buffer.pointer, + obj->buffer.length); + if (ret) dev_err(dev, "failed to merge updated NFIT\n"); - } } else { /* Bad _FIT, restore old nfit */ dev_err(dev, "Invalid _FIT\n"); diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h index 80fb2c0ac8bf..5179eba97a47 100644 --- a/drivers/acpi/nfit.h +++ b/drivers/acpi/nfit.h @@ -135,7 +135,6 @@ struct nfit_mem { struct acpi_nfit_desc { struct nvdimm_bus_descriptor nd_desc; struct acpi_table_header acpi_header; - struct acpi_nfit_header *nfit; struct mutex init_mutex; struct list_head memdevs; struct list_head flushes; @@ -201,6 +200,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc( } const u8 *to_nfit_uuid(enum nfit_uuids id); -int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz); +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz); void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev); #endif /* __NFIT_H__ */ diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index ff09a28890ed..52df3c20231d 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -1458,7 +1458,6 @@ static int nfit_test_probe(struct platform_device *pdev) nfit_test->setup(nfit_test); acpi_desc = &nfit_test->acpi_desc; acpi_nfit_desc_init(acpi_desc, &pdev->dev); - acpi_desc->nfit = nfit_test->nfit_buf; acpi_desc->blk_do_io = nfit_test_blk_do_io; nd_desc = &acpi_desc->nd_desc; nd_desc->provider_name = NULL; @@ -1467,7 +1466,8 @@ static int nfit_test_probe(struct platform_device *pdev) if (!acpi_desc->nvdimm_bus) return -ENXIO; - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, + nfit_test->nfit_size); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); return rc; @@ -1479,7 +1479,8 @@ static int nfit_test_probe(struct platform_device *pdev) nfit_test->setup_hotplug = 1; nfit_test->setup(nfit_test); - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, + nfit_test->nfit_size); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); return rc; _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/2] nfit: cleanup acpi_nfit_init calling convention @ 2016-07-15 3:29 ` Dan Williams 0 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 3:29 UTC (permalink / raw) To: linux-nvdimm; +Cc: linux-acpi Pass the nfit buffer as a parameter rather than hanging it off of acpi_desc. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/acpi/nfit.c | 56 +++++++++++++------------------------- drivers/acpi/nfit.h | 3 +- tools/testing/nvdimm/test/nfit.c | 7 +++-- 3 files changed, 24 insertions(+), 42 deletions(-) diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c index 008dbaaa2b75..a04f2486e1ca 100644 --- a/drivers/acpi/nfit.c +++ b/drivers/acpi/nfit.c @@ -2224,12 +2224,11 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc, return 0; } -int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size sz) { struct device *dev = acpi_desc->dev; struct nfit_table_prev prev; const void *end; - u8 *data; int rc; mutex_lock(&acpi_desc->init_mutex); @@ -2254,7 +2253,6 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) list_cut_position(&prev.flushes, &acpi_desc->flushes, acpi_desc->flushes.prev); - data = (u8 *) acpi_desc->nfit; end = data + sz; while (!IS_ERR_OR_NULL(data)) data = add_table(acpi_desc, &prev, data, end); @@ -2377,7 +2375,7 @@ static int acpi_nfit_add(struct acpi_device *adev) struct acpi_table_header *tbl; acpi_status status = AE_OK; acpi_size sz; - int rc; + int rc = 0; status = acpi_get_table_with_size(ACPI_SIG_NFIT, 0, &tbl, &sz); if (ACPI_FAILURE(status)) { @@ -2394,40 +2392,30 @@ static int acpi_nfit_add(struct acpi_device *adev) if (!acpi_desc->nvdimm_bus) return -ENOMEM; - /* - * Save the acpi header for later and then skip it, - * making nfit point to the first nfit table header. - */ + /* Save the acpi header for exporting the revision via sysfs */ acpi_desc->acpi_header = *tbl; - acpi_desc->nfit = (void *) tbl + sizeof(struct acpi_table_nfit); - sz -= sizeof(struct acpi_table_nfit); /* Evaluate _FIT and override with that if present */ status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); if (ACPI_SUCCESS(status) && buf.length > 0) { - union acpi_object *obj; - /* - * Adjust for the acpi_object header of the _FIT - */ - obj = buf.pointer; - if (obj->type == ACPI_TYPE_BUFFER) { - acpi_desc->nfit = - (struct acpi_nfit_header *)obj->buffer.pointer; - sz = obj->buffer.length; - rc = acpi_nfit_init(acpi_desc, sz); - } else + union acpi_object *obj = buf.pointer; + + if (obj->type == ACPI_TYPE_BUFFER) + rc = acpi_nfit_init(acpi_desc, obj->buffer.pointer, + obj->buffer.length); + else dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", __func__, (int) obj->type); kfree(buf.pointer); - acpi_desc->nfit = NULL; } else - rc = acpi_nfit_init(acpi_desc, sz); + /* skip over the lead-in header table */ + rc = acpi_nfit_init(acpi_desc, (void *) tbl + + sizeof(struct acpi_table_nfit), + sz - sizeof(struct acpi_table_nfit)); - if (rc) { + if (rc) nvdimm_bus_unregister(acpi_desc->nvdimm_bus); - return rc; - } - return 0; + return rc; } static int acpi_nfit_remove(struct acpi_device *adev) @@ -2444,9 +2432,8 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) { struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev); struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; - struct acpi_nfit_header *nfit_saved; - union acpi_object *obj; struct device *dev = &adev->dev; + union acpi_object *obj; acpi_status status; int ret; @@ -2482,17 +2469,12 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) goto out_unlock; } - nfit_saved = acpi_desc->nfit; obj = buf.pointer; if (obj->type == ACPI_TYPE_BUFFER) { - acpi_desc->nfit = - (struct acpi_nfit_header *)obj->buffer.pointer; - ret = acpi_nfit_init(acpi_desc, obj->buffer.length); - if (ret) { - /* Merge failed, restore old nfit, and exit */ - acpi_desc->nfit = nfit_saved; + ret = acpi_nfit_init(acpi_desc, obj->buffer.pointer, + obj->buffer.length); + if (ret) dev_err(dev, "failed to merge updated NFIT\n"); - } } else { /* Bad _FIT, restore old nfit */ dev_err(dev, "Invalid _FIT\n"); diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h index 80fb2c0ac8bf..5179eba97a47 100644 --- a/drivers/acpi/nfit.h +++ b/drivers/acpi/nfit.h @@ -135,7 +135,6 @@ struct nfit_mem { struct acpi_nfit_desc { struct nvdimm_bus_descriptor nd_desc; struct acpi_table_header acpi_header; - struct acpi_nfit_header *nfit; struct mutex init_mutex; struct list_head memdevs; struct list_head flushes; @@ -201,6 +200,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc( } const u8 *to_nfit_uuid(enum nfit_uuids id); -int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz); +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz); void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev); #endif /* __NFIT_H__ */ diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c index ff09a28890ed..52df3c20231d 100644 --- a/tools/testing/nvdimm/test/nfit.c +++ b/tools/testing/nvdimm/test/nfit.c @@ -1458,7 +1458,6 @@ static int nfit_test_probe(struct platform_device *pdev) nfit_test->setup(nfit_test); acpi_desc = &nfit_test->acpi_desc; acpi_nfit_desc_init(acpi_desc, &pdev->dev); - acpi_desc->nfit = nfit_test->nfit_buf; acpi_desc->blk_do_io = nfit_test_blk_do_io; nd_desc = &acpi_desc->nd_desc; nd_desc->provider_name = NULL; @@ -1467,7 +1466,8 @@ static int nfit_test_probe(struct platform_device *pdev) if (!acpi_desc->nvdimm_bus) return -ENXIO; - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, + nfit_test->nfit_size); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); return rc; @@ -1479,7 +1479,8 @@ static int nfit_test_probe(struct platform_device *pdev) nfit_test->setup_hotplug = 1; nfit_test->setup(nfit_test); - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, + nfit_test->nfit_size); if (rc) { nvdimm_bus_unregister(acpi_desc->nvdimm_bus); return rc; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] nfit: cleanup acpi_nfit_init calling convention @ 2016-07-15 8:40 ` joeyli 0 siblings, 0 replies; 21+ messages in thread From: joeyli @ 2016-07-15 8:40 UTC (permalink / raw) To: Dan Williams; +Cc: linux-acpi, linux-nvdimm On Thu, Jul 14, 2016 at 08:29:02PM -0700, Dan Williams wrote: > Pass the nfit buffer as a parameter rather than hanging it off of > acpi_desc. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Reviewed-by: Lee, Chun-Yi <jlee@suse.com> Thanks a lot! Joey Lee > --- > drivers/acpi/nfit.c | 56 +++++++++++++------------------------- > drivers/acpi/nfit.h | 3 +- > tools/testing/nvdimm/test/nfit.c | 7 +++-- > 3 files changed, 24 insertions(+), 42 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 008dbaaa2b75..a04f2486e1ca 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2224,12 +2224,11 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc, > return 0; > } > > -int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) > +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size sz) > { > struct device *dev = acpi_desc->dev; > struct nfit_table_prev prev; > const void *end; > - u8 *data; > int rc; > > mutex_lock(&acpi_desc->init_mutex); > @@ -2254,7 +2253,6 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) > list_cut_position(&prev.flushes, &acpi_desc->flushes, > acpi_desc->flushes.prev); > > - data = (u8 *) acpi_desc->nfit; > end = data + sz; > while (!IS_ERR_OR_NULL(data)) > data = add_table(acpi_desc, &prev, data, end); > @@ -2377,7 +2375,7 @@ static int acpi_nfit_add(struct acpi_device *adev) > struct acpi_table_header *tbl; > acpi_status status = AE_OK; > acpi_size sz; > - int rc; > + int rc = 0; > > status = acpi_get_table_with_size(ACPI_SIG_NFIT, 0, &tbl, &sz); > if (ACPI_FAILURE(status)) { > @@ -2394,40 +2392,30 @@ static int acpi_nfit_add(struct acpi_device *adev) > if (!acpi_desc->nvdimm_bus) > return -ENOMEM; > > - /* > - * Save the acpi header for later and then skip it, > - * making nfit point to the first nfit table header. > - */ > + /* Save the acpi header for exporting the revision via sysfs */ > acpi_desc->acpi_header = *tbl; > - acpi_desc->nfit = (void *) tbl + sizeof(struct acpi_table_nfit); > - sz -= sizeof(struct acpi_table_nfit); > > /* Evaluate _FIT and override with that if present */ > status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); > if (ACPI_SUCCESS(status) && buf.length > 0) { > - union acpi_object *obj; > - /* > - * Adjust for the acpi_object header of the _FIT > - */ > - obj = buf.pointer; > - if (obj->type == ACPI_TYPE_BUFFER) { > - acpi_desc->nfit = > - (struct acpi_nfit_header *)obj->buffer.pointer; > - sz = obj->buffer.length; > - rc = acpi_nfit_init(acpi_desc, sz); > - } else > + union acpi_object *obj = buf.pointer; > + > + if (obj->type == ACPI_TYPE_BUFFER) > + rc = acpi_nfit_init(acpi_desc, obj->buffer.pointer, > + obj->buffer.length); > + else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); > kfree(buf.pointer); > - acpi_desc->nfit = NULL; > } else > - rc = acpi_nfit_init(acpi_desc, sz); > + /* skip over the lead-in header table */ > + rc = acpi_nfit_init(acpi_desc, (void *) tbl > + + sizeof(struct acpi_table_nfit), > + sz - sizeof(struct acpi_table_nfit)); > > - if (rc) { > + if (rc) > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > - return rc; > - } > - return 0; > + return rc; > } > > static int acpi_nfit_remove(struct acpi_device *adev) > @@ -2444,9 +2432,8 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) > { > struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev); > struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; > - struct acpi_nfit_header *nfit_saved; > - union acpi_object *obj; > struct device *dev = &adev->dev; > + union acpi_object *obj; > acpi_status status; > int ret; > > @@ -2482,17 +2469,12 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) > goto out_unlock; > } > > - nfit_saved = acpi_desc->nfit; > obj = buf.pointer; > if (obj->type == ACPI_TYPE_BUFFER) { > - acpi_desc->nfit = > - (struct acpi_nfit_header *)obj->buffer.pointer; > - ret = acpi_nfit_init(acpi_desc, obj->buffer.length); > - if (ret) { > - /* Merge failed, restore old nfit, and exit */ > - acpi_desc->nfit = nfit_saved; > + ret = acpi_nfit_init(acpi_desc, obj->buffer.pointer, > + obj->buffer.length); > + if (ret) > dev_err(dev, "failed to merge updated NFIT\n"); > - } > } else { > /* Bad _FIT, restore old nfit */ > dev_err(dev, "Invalid _FIT\n"); > diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h > index 80fb2c0ac8bf..5179eba97a47 100644 > --- a/drivers/acpi/nfit.h > +++ b/drivers/acpi/nfit.h > @@ -135,7 +135,6 @@ struct nfit_mem { > struct acpi_nfit_desc { > struct nvdimm_bus_descriptor nd_desc; > struct acpi_table_header acpi_header; > - struct acpi_nfit_header *nfit; > struct mutex init_mutex; > struct list_head memdevs; > struct list_head flushes; > @@ -201,6 +200,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc( > } > > const u8 *to_nfit_uuid(enum nfit_uuids id); > -int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz); > +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz); > void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev); > #endif /* __NFIT_H__ */ > diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c > index ff09a28890ed..52df3c20231d 100644 > --- a/tools/testing/nvdimm/test/nfit.c > +++ b/tools/testing/nvdimm/test/nfit.c > @@ -1458,7 +1458,6 @@ static int nfit_test_probe(struct platform_device *pdev) > nfit_test->setup(nfit_test); > acpi_desc = &nfit_test->acpi_desc; > acpi_nfit_desc_init(acpi_desc, &pdev->dev); > - acpi_desc->nfit = nfit_test->nfit_buf; > acpi_desc->blk_do_io = nfit_test_blk_do_io; > nd_desc = &acpi_desc->nd_desc; > nd_desc->provider_name = NULL; > @@ -1467,7 +1466,8 @@ static int nfit_test_probe(struct platform_device *pdev) > if (!acpi_desc->nvdimm_bus) > return -ENXIO; > > - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); > + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, > + nfit_test->nfit_size); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > @@ -1479,7 +1479,8 @@ static int nfit_test_probe(struct platform_device *pdev) > nfit_test->setup_hotplug = 1; > nfit_test->setup(nfit_test); > > - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); > + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, > + nfit_test->nfit_size); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] nfit: cleanup acpi_nfit_init calling convention @ 2016-07-15 8:40 ` joeyli 0 siblings, 0 replies; 21+ messages in thread From: joeyli @ 2016-07-15 8:40 UTC (permalink / raw) To: Dan Williams Cc: linux-acpi-u79uwXL29TY76Z2rM5mHXA, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw On Thu, Jul 14, 2016 at 08:29:02PM -0700, Dan Williams wrote: > Pass the nfit buffer as a parameter rather than hanging it off of > acpi_desc. > > Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Reviewed-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org> Thanks a lot! Joey Lee > --- > drivers/acpi/nfit.c | 56 +++++++++++++------------------------- > drivers/acpi/nfit.h | 3 +- > tools/testing/nvdimm/test/nfit.c | 7 +++-- > 3 files changed, 24 insertions(+), 42 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 008dbaaa2b75..a04f2486e1ca 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2224,12 +2224,11 @@ static int acpi_nfit_check_deletions(struct acpi_nfit_desc *acpi_desc, > return 0; > } > > -int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) > +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *data, acpi_size sz) > { > struct device *dev = acpi_desc->dev; > struct nfit_table_prev prev; > const void *end; > - u8 *data; > int rc; > > mutex_lock(&acpi_desc->init_mutex); > @@ -2254,7 +2253,6 @@ int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, acpi_size sz) > list_cut_position(&prev.flushes, &acpi_desc->flushes, > acpi_desc->flushes.prev); > > - data = (u8 *) acpi_desc->nfit; > end = data + sz; > while (!IS_ERR_OR_NULL(data)) > data = add_table(acpi_desc, &prev, data, end); > @@ -2377,7 +2375,7 @@ static int acpi_nfit_add(struct acpi_device *adev) > struct acpi_table_header *tbl; > acpi_status status = AE_OK; > acpi_size sz; > - int rc; > + int rc = 0; > > status = acpi_get_table_with_size(ACPI_SIG_NFIT, 0, &tbl, &sz); > if (ACPI_FAILURE(status)) { > @@ -2394,40 +2392,30 @@ static int acpi_nfit_add(struct acpi_device *adev) > if (!acpi_desc->nvdimm_bus) > return -ENOMEM; > > - /* > - * Save the acpi header for later and then skip it, > - * making nfit point to the first nfit table header. > - */ > + /* Save the acpi header for exporting the revision via sysfs */ > acpi_desc->acpi_header = *tbl; > - acpi_desc->nfit = (void *) tbl + sizeof(struct acpi_table_nfit); > - sz -= sizeof(struct acpi_table_nfit); > > /* Evaluate _FIT and override with that if present */ > status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); > if (ACPI_SUCCESS(status) && buf.length > 0) { > - union acpi_object *obj; > - /* > - * Adjust for the acpi_object header of the _FIT > - */ > - obj = buf.pointer; > - if (obj->type == ACPI_TYPE_BUFFER) { > - acpi_desc->nfit = > - (struct acpi_nfit_header *)obj->buffer.pointer; > - sz = obj->buffer.length; > - rc = acpi_nfit_init(acpi_desc, sz); > - } else > + union acpi_object *obj = buf.pointer; > + > + if (obj->type == ACPI_TYPE_BUFFER) > + rc = acpi_nfit_init(acpi_desc, obj->buffer.pointer, > + obj->buffer.length); > + else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); > kfree(buf.pointer); > - acpi_desc->nfit = NULL; > } else > - rc = acpi_nfit_init(acpi_desc, sz); > + /* skip over the lead-in header table */ > + rc = acpi_nfit_init(acpi_desc, (void *) tbl > + + sizeof(struct acpi_table_nfit), > + sz - sizeof(struct acpi_table_nfit)); > > - if (rc) { > + if (rc) > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > - return rc; > - } > - return 0; > + return rc; > } > > static int acpi_nfit_remove(struct acpi_device *adev) > @@ -2444,9 +2432,8 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) > { > struct acpi_nfit_desc *acpi_desc = dev_get_drvdata(&adev->dev); > struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; > - struct acpi_nfit_header *nfit_saved; > - union acpi_object *obj; > struct device *dev = &adev->dev; > + union acpi_object *obj; > acpi_status status; > int ret; > > @@ -2482,17 +2469,12 @@ static void acpi_nfit_notify(struct acpi_device *adev, u32 event) > goto out_unlock; > } > > - nfit_saved = acpi_desc->nfit; > obj = buf.pointer; > if (obj->type == ACPI_TYPE_BUFFER) { > - acpi_desc->nfit = > - (struct acpi_nfit_header *)obj->buffer.pointer; > - ret = acpi_nfit_init(acpi_desc, obj->buffer.length); > - if (ret) { > - /* Merge failed, restore old nfit, and exit */ > - acpi_desc->nfit = nfit_saved; > + ret = acpi_nfit_init(acpi_desc, obj->buffer.pointer, > + obj->buffer.length); > + if (ret) > dev_err(dev, "failed to merge updated NFIT\n"); > - } > } else { > /* Bad _FIT, restore old nfit */ > dev_err(dev, "Invalid _FIT\n"); > diff --git a/drivers/acpi/nfit.h b/drivers/acpi/nfit.h > index 80fb2c0ac8bf..5179eba97a47 100644 > --- a/drivers/acpi/nfit.h > +++ b/drivers/acpi/nfit.h > @@ -135,7 +135,6 @@ struct nfit_mem { > struct acpi_nfit_desc { > struct nvdimm_bus_descriptor nd_desc; > struct acpi_table_header acpi_header; > - struct acpi_nfit_header *nfit; > struct mutex init_mutex; > struct list_head memdevs; > struct list_head flushes; > @@ -201,6 +200,6 @@ static inline struct acpi_nfit_desc *to_acpi_desc( > } > > const u8 *to_nfit_uuid(enum nfit_uuids id); > -int acpi_nfit_init(struct acpi_nfit_desc *nfit, acpi_size sz); > +int acpi_nfit_init(struct acpi_nfit_desc *acpi_desc, void *nfit, acpi_size sz); > void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev); > #endif /* __NFIT_H__ */ > diff --git a/tools/testing/nvdimm/test/nfit.c b/tools/testing/nvdimm/test/nfit.c > index ff09a28890ed..52df3c20231d 100644 > --- a/tools/testing/nvdimm/test/nfit.c > +++ b/tools/testing/nvdimm/test/nfit.c > @@ -1458,7 +1458,6 @@ static int nfit_test_probe(struct platform_device *pdev) > nfit_test->setup(nfit_test); > acpi_desc = &nfit_test->acpi_desc; > acpi_nfit_desc_init(acpi_desc, &pdev->dev); > - acpi_desc->nfit = nfit_test->nfit_buf; > acpi_desc->blk_do_io = nfit_test_blk_do_io; > nd_desc = &acpi_desc->nd_desc; > nd_desc->provider_name = NULL; > @@ -1467,7 +1466,8 @@ static int nfit_test_probe(struct platform_device *pdev) > if (!acpi_desc->nvdimm_bus) > return -ENXIO; > > - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); > + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, > + nfit_test->nfit_size); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > @@ -1479,7 +1479,8 @@ static int nfit_test_probe(struct platform_device *pdev) > nfit_test->setup_hotplug = 1; > nfit_test->setup(nfit_test); > > - rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_size); > + rc = acpi_nfit_init(acpi_desc, nfit_test->nfit_buf, > + nfit_test->nfit_size); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 5:15 ` joeyli 0 siblings, 0 replies; 21+ messages in thread From: joeyli @ 2016-07-15 5:15 UTC (permalink / raw) To: Dan Williams; +Cc: Xiao Guangrong, linux-nvdimm, stable, linux-acpi Hi Dan, On Thu, Jul 14, 2016 at 08:28:57PM -0700, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > > Cc: <stable@vger.kernel.org> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/acpi/nfit.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 0497175ee6cb..008dbaaa2b75 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > sz = obj->buffer.length; > + rc = acpi_nfit_init(acpi_desc, sz); > } else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); > - } > + kfree(buf.pointer); > + acpi_desc->nfit = NULL; Looks "acpi_desc->nfit = NULL" statement will be removed in [PATCH 2/2] immediately. Why add it in PATCH 1? > + } else > + rc = acpi_nfit_init(acpi_desc, sz); > > - rc = acpi_nfit_init(acpi_desc, sz); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > Other parts are no problem to me. Thanks a lot! Joey Lee _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 5:15 ` joeyli 0 siblings, 0 replies; 21+ messages in thread From: joeyli @ 2016-07-15 5:15 UTC (permalink / raw) To: Dan Williams Cc: linux-nvdimm, Vishal Verma, linux-acpi, stable, Xiao Guangrong, Haozhong Zhang Hi Dan, On Thu, Jul 14, 2016 at 08:28:57PM -0700, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > > Cc: <stable@vger.kernel.org> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/acpi/nfit.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 0497175ee6cb..008dbaaa2b75 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > sz = obj->buffer.length; > + rc = acpi_nfit_init(acpi_desc, sz); > } else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); > - } > + kfree(buf.pointer); > + acpi_desc->nfit = NULL; Looks "acpi_desc->nfit = NULL" statement will be removed in [PATCH 2/2] immediately. Why add it in PATCH 1? > + } else > + rc = acpi_nfit_init(acpi_desc, sz); > > - rc = acpi_nfit_init(acpi_desc, sz); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > Other parts are no problem to me. Thanks a lot! Joey Lee ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 5:15 ` joeyli 0 siblings, 0 replies; 21+ messages in thread From: joeyli @ 2016-07-15 5:15 UTC (permalink / raw) To: Dan Williams Cc: Xiao Guangrong, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, stable-u79uwXL29TY76Z2rM5mHXA, linux-acpi-u79uwXL29TY76Z2rM5mHXA Hi Dan, On Thu, Jul 14, 2016 at 08:28:57PM -0700, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > > Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> > Cc: Vishal Verma <vishal.l.verma-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > Reported-by: Xiao Guangrong <guangrong.xiao-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > Reported-by: Haozhong Zhang <haozhong.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > Signed-off-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> > --- > drivers/acpi/nfit.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 0497175ee6cb..008dbaaa2b75 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > sz = obj->buffer.length; > + rc = acpi_nfit_init(acpi_desc, sz); > } else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); > - } > + kfree(buf.pointer); > + acpi_desc->nfit = NULL; Looks "acpi_desc->nfit = NULL" statement will be removed in [PATCH 2/2] immediately. Why add it in PATCH 1? > + } else > + rc = acpi_nfit_init(acpi_desc, sz); > > - rc = acpi_nfit_init(acpi_desc, sz); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > Other parts are no problem to me. Thanks a lot! Joey Lee ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak 2016-07-15 5:15 ` joeyli @ 2016-07-15 5:27 ` Dan Williams -1 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 5:27 UTC (permalink / raw) To: joeyli; +Cc: Xiao Guangrong, linux-nvdimm, stable, Linux ACPI On Thu, Jul 14, 2016 at 10:15 PM, joeyli <jlee@suse.com> wrote: > Hi Dan, > > On Thu, Jul 14, 2016 at 08:28:57PM -0700, Dan Williams wrote: >> acpi_evaluate_object() allocates memory. Free the buffer allocated >> during acpi_nfit_add(). >> >> Cc: <stable@vger.kernel.org> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> >> Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/acpi/nfit.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> index 0497175ee6cb..008dbaaa2b75 100644 >> --- a/drivers/acpi/nfit.c >> +++ b/drivers/acpi/nfit.c >> @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) >> acpi_desc->nfit = >> (struct acpi_nfit_header *)obj->buffer.pointer; >> sz = obj->buffer.length; >> + rc = acpi_nfit_init(acpi_desc, sz); >> } else >> dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", >> __func__, (int) obj->type); >> - } >> + kfree(buf.pointer); >> + acpi_desc->nfit = NULL; > > Looks "acpi_desc->nfit = NULL" statement will be removed in [PATCH 2/2] > immediately. Why add it in PATCH 1? I was debating it, but for code readability of -stable kernels (where patch2 will not be included) I want to make it clear that nothing uses the value of ->nfit outside of acpi_nfit_init(). > >> + } else >> + rc = acpi_nfit_init(acpi_desc, sz); >> >> - rc = acpi_nfit_init(acpi_desc, sz); >> if (rc) { >> nvdimm_bus_unregister(acpi_desc->nvdimm_bus); >> return rc; >> > > Other parts are no problem to me. Thanks. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 5:27 ` Dan Williams 0 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 5:27 UTC (permalink / raw) To: joeyli Cc: linux-nvdimm, Vishal Verma, Linux ACPI, stable, Xiao Guangrong, Haozhong Zhang On Thu, Jul 14, 2016 at 10:15 PM, joeyli <jlee@suse.com> wrote: > Hi Dan, > > On Thu, Jul 14, 2016 at 08:28:57PM -0700, Dan Williams wrote: >> acpi_evaluate_object() allocates memory. Free the buffer allocated >> during acpi_nfit_add(). >> >> Cc: <stable@vger.kernel.org> >> Cc: Vishal Verma <vishal.l.verma@intel.com> >> Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> >> Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/acpi/nfit.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> index 0497175ee6cb..008dbaaa2b75 100644 >> --- a/drivers/acpi/nfit.c >> +++ b/drivers/acpi/nfit.c >> @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) >> acpi_desc->nfit = >> (struct acpi_nfit_header *)obj->buffer.pointer; >> sz = obj->buffer.length; >> + rc = acpi_nfit_init(acpi_desc, sz); >> } else >> dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", >> __func__, (int) obj->type); >> - } >> + kfree(buf.pointer); >> + acpi_desc->nfit = NULL; > > Looks "acpi_desc->nfit = NULL" statement will be removed in [PATCH 2/2] > immediately. Why add it in PATCH 1? I was debating it, but for code readability of -stable kernels (where patch2 will not be included) I want to make it clear that nothing uses the value of ->nfit outside of acpi_nfit_init(). > >> + } else >> + rc = acpi_nfit_init(acpi_desc, sz); >> >> - rc = acpi_nfit_init(acpi_desc, sz); >> if (rc) { >> nvdimm_bus_unregister(acpi_desc->nvdimm_bus); >> return rc; >> > > Other parts are no problem to me. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak 2016-07-15 3:28 ` Dan Williams @ 2016-07-15 5:47 ` Xiao Guangrong -1 siblings, 0 replies; 21+ messages in thread From: Xiao Guangrong @ 2016-07-15 5:47 UTC (permalink / raw) To: Dan Williams, linux-nvdimm; +Cc: stable, linux-acpi On 07/15/2016 11:28 AM, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > Dan, thanks for your fix. Another one is the use-after-free issue in acpi_nfit_notify(): /* Evaluate _FIT */ status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); ... acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; ... kfree(buf.pointer); _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 5:47 ` Xiao Guangrong 0 siblings, 0 replies; 21+ messages in thread From: Xiao Guangrong @ 2016-07-15 5:47 UTC (permalink / raw) To: Dan Williams, linux-nvdimm Cc: Vishal Verma, linux-acpi, stable, Haozhong Zhang On 07/15/2016 11:28 AM, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > Dan, thanks for your fix. Another one is the use-after-free issue in acpi_nfit_notify(): /* Evaluate _FIT */ status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); ... acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; ... kfree(buf.pointer); ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak 2016-07-15 5:47 ` Xiao Guangrong @ 2016-07-15 14:57 ` Dan Williams -1 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 14:57 UTC (permalink / raw) To: Xiao Guangrong; +Cc: Linux ACPI, stable, linux-nvdimm On Thu, Jul 14, 2016 at 10:47 PM, Xiao Guangrong <guangrong.xiao@intel.com> wrote: > > > On 07/15/2016 11:28 AM, Dan Williams wrote: >> >> acpi_evaluate_object() allocates memory. Free the buffer allocated >> during acpi_nfit_add(). >> > > Dan, thanks for your fix. > > Another one is the use-after-free issue in acpi_nfit_notify(): > > /* Evaluate _FIT */ > status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); > ... > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > ... > kfree(buf.pointer); grep for acpi_desc->nfit usages, there are no usages after acpi_nfit_init(). We go through the hassle of setting up nfit_saved for no reason. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 14:57 ` Dan Williams 0 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 14:57 UTC (permalink / raw) To: Xiao Guangrong Cc: linux-nvdimm, Vishal Verma, Linux ACPI, stable, Haozhong Zhang On Thu, Jul 14, 2016 at 10:47 PM, Xiao Guangrong <guangrong.xiao@intel.com> wrote: > > > On 07/15/2016 11:28 AM, Dan Williams wrote: >> >> acpi_evaluate_object() allocates memory. Free the buffer allocated >> during acpi_nfit_add(). >> > > Dan, thanks for your fix. > > Another one is the use-after-free issue in acpi_nfit_notify(): > > /* Evaluate _FIT */ > status = acpi_evaluate_object(adev->handle, "_FIT", NULL, &buf); > ... > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > ... > kfree(buf.pointer); grep for acpi_desc->nfit usages, there are no usages after acpi_nfit_init(). We go through the hassle of setting up nfit_saved for no reason. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak 2016-07-15 3:28 ` Dan Williams @ 2016-07-15 7:55 ` Haozhong Zhang -1 siblings, 0 replies; 21+ messages in thread From: Haozhong Zhang @ 2016-07-15 7:55 UTC (permalink / raw) To: Dan Williams; +Cc: Xiao Guangrong, linux-acpi, stable, linux-nvdimm On 07/14/16 20:28, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > > Cc: <stable@vger.kernel.org> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/acpi/nfit.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 0497175ee6cb..008dbaaa2b75 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > sz = obj->buffer.length; > + rc = acpi_nfit_init(acpi_desc, sz); > } else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. Should we set it to a non-zero value in this path? > - } > + kfree(buf.pointer); > + acpi_desc->nfit = NULL; I notice the following code in acpi_nfit_notify(): nfit_saved = acpi_desc->nfit; obj = buf.pointer; if (obj->type == ACPI_TYPE_BUFFER) { acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; ret = acpi_nfit_init(acpi_desc, obj->buffer.length); if (ret) { /* Merge failed, restore old nfit, and exit */ acpi_desc->nfit = nfit_saved; dev_err(dev, "failed to merge updated NFIT\n"); } ... If we set acpi_desc->nfit to NULL in acpi_nfit_add() and acpi_nfit_init() in acpi_nfit_notify() fails, it will be impossible to restore the old nfit, because nfit_saved is NULL. Thanks, Haozhong > + } else > + rc = acpi_nfit_init(acpi_desc, sz); > > - rc = acpi_nfit_init(acpi_desc, sz); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 7:55 ` Haozhong Zhang 0 siblings, 0 replies; 21+ messages in thread From: Haozhong Zhang @ 2016-07-15 7:55 UTC (permalink / raw) To: Dan Williams Cc: linux-nvdimm, Vishal Verma, linux-acpi, stable, Xiao Guangrong On 07/14/16 20:28, Dan Williams wrote: > acpi_evaluate_object() allocates memory. Free the buffer allocated > during acpi_nfit_add(). > > Cc: <stable@vger.kernel.org> > Cc: Vishal Verma <vishal.l.verma@intel.com> > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/acpi/nfit.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > index 0497175ee6cb..008dbaaa2b75 100644 > --- a/drivers/acpi/nfit.c > +++ b/drivers/acpi/nfit.c > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > acpi_desc->nfit = > (struct acpi_nfit_header *)obj->buffer.pointer; > sz = obj->buffer.length; > + rc = acpi_nfit_init(acpi_desc, sz); > } else > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > __func__, (int) obj->type); 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. Should we set it to a non-zero value in this path? > - } > + kfree(buf.pointer); > + acpi_desc->nfit = NULL; I notice the following code in acpi_nfit_notify(): nfit_saved = acpi_desc->nfit; obj = buf.pointer; if (obj->type == ACPI_TYPE_BUFFER) { acpi_desc->nfit = (struct acpi_nfit_header *)obj->buffer.pointer; ret = acpi_nfit_init(acpi_desc, obj->buffer.length); if (ret) { /* Merge failed, restore old nfit, and exit */ acpi_desc->nfit = nfit_saved; dev_err(dev, "failed to merge updated NFIT\n"); } ... If we set acpi_desc->nfit to NULL in acpi_nfit_add() and acpi_nfit_init() in acpi_nfit_notify() fails, it will be impossible to restore the old nfit, because nfit_saved is NULL. Thanks, Haozhong > + } else > + rc = acpi_nfit_init(acpi_desc, sz); > > - rc = acpi_nfit_init(acpi_desc, sz); > if (rc) { > nvdimm_bus_unregister(acpi_desc->nvdimm_bus); > return rc; > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak 2016-07-15 7:55 ` Haozhong Zhang @ 2016-07-15 8:12 ` Haozhong Zhang -1 siblings, 0 replies; 21+ messages in thread From: Haozhong Zhang @ 2016-07-15 8:12 UTC (permalink / raw) To: Dan Williams, linux-nvdimm, Vishal Verma, linux-acpi, stable, Xiao Guangrong On 07/15/16 15:55, Haozhong Zhang wrote: > On 07/14/16 20:28, Dan Williams wrote: > > acpi_evaluate_object() allocates memory. Free the buffer allocated > > during acpi_nfit_add(). > > > > Cc: <stable@vger.kernel.org> > > Cc: Vishal Verma <vishal.l.verma@intel.com> > > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> > > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/acpi/nfit.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > > index 0497175ee6cb..008dbaaa2b75 100644 > > --- a/drivers/acpi/nfit.c > > +++ b/drivers/acpi/nfit.c > > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > > acpi_desc->nfit = > > (struct acpi_nfit_header *)obj->buffer.pointer; > > sz = obj->buffer.length; > > + rc = acpi_nfit_init(acpi_desc, sz); > > } else > > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > > __func__, (int) obj->type); > > 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. > Should we set it to a non-zero value in this path? 'rc' should be set to 0 here, as what patch 2 does. Sorry for my mistake. Haozhong _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 8:12 ` Haozhong Zhang 0 siblings, 0 replies; 21+ messages in thread From: Haozhong Zhang @ 2016-07-15 8:12 UTC (permalink / raw) To: Dan Williams, linux-nvdimm, Vishal Verma, linux-acpi, stable, Xiao Guangrong On 07/15/16 15:55, Haozhong Zhang wrote: > On 07/14/16 20:28, Dan Williams wrote: > > acpi_evaluate_object() allocates memory. Free the buffer allocated > > during acpi_nfit_add(). > > > > Cc: <stable@vger.kernel.org> > > Cc: Vishal Verma <vishal.l.verma@intel.com> > > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> > > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > > --- > > drivers/acpi/nfit.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c > > index 0497175ee6cb..008dbaaa2b75 100644 > > --- a/drivers/acpi/nfit.c > > +++ b/drivers/acpi/nfit.c > > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) > > acpi_desc->nfit = > > (struct acpi_nfit_header *)obj->buffer.pointer; > > sz = obj->buffer.length; > > + rc = acpi_nfit_init(acpi_desc, sz); > > } else > > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", > > __func__, (int) obj->type); > > 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. > Should we set it to a non-zero value in this path? 'rc' should be set to 0 here, as what patch 2 does. Sorry for my mistake. Haozhong ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak 2016-07-15 8:12 ` Haozhong Zhang @ 2016-07-15 17:10 ` Dan Williams -1 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 17:10 UTC (permalink / raw) To: Dan Williams, linux-nvdimm, Vishal Verma, Linux ACPI, stable, Xiao Guangrong On Fri, Jul 15, 2016 at 1:12 AM, Haozhong Zhang <haozhong.zhang@intel.com> wrote: > On 07/15/16 15:55, Haozhong Zhang wrote: >> On 07/14/16 20:28, Dan Williams wrote: >> > acpi_evaluate_object() allocates memory. Free the buffer allocated >> > during acpi_nfit_add(). >> > >> > Cc: <stable@vger.kernel.org> >> > Cc: Vishal Verma <vishal.l.verma@intel.com> >> > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> >> > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> >> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> > --- >> > drivers/acpi/nfit.c | 7 +++++-- >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> > index 0497175ee6cb..008dbaaa2b75 100644 >> > --- a/drivers/acpi/nfit.c >> > +++ b/drivers/acpi/nfit.c >> > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) >> > acpi_desc->nfit = >> > (struct acpi_nfit_header *)obj->buffer.pointer; >> > sz = obj->buffer.length; >> > + rc = acpi_nfit_init(acpi_desc, sz); >> > } else >> > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", >> > __func__, (int) obj->type); >> >> 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. >> Should we set it to a non-zero value in this path? > > 'rc' should be set to 0 here, as what patch 2 does. Sorry for my mistake. No, this is good feedback because patch1 is targeted for -stable. Will fix, thanks! _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] nfit: fix _FIT evaluation memory leak @ 2016-07-15 17:10 ` Dan Williams 0 siblings, 0 replies; 21+ messages in thread From: Dan Williams @ 2016-07-15 17:10 UTC (permalink / raw) To: Dan Williams, linux-nvdimm, Vishal Verma, Linux ACPI, stable, Xiao Guangrong On Fri, Jul 15, 2016 at 1:12 AM, Haozhong Zhang <haozhong.zhang@intel.com> wrote: > On 07/15/16 15:55, Haozhong Zhang wrote: >> On 07/14/16 20:28, Dan Williams wrote: >> > acpi_evaluate_object() allocates memory. Free the buffer allocated >> > during acpi_nfit_add(). >> > >> > Cc: <stable@vger.kernel.org> >> > Cc: Vishal Verma <vishal.l.verma@intel.com> >> > Reported-by: Xiao Guangrong <guangrong.xiao@intel.com> >> > Reported-by: Haozhong Zhang <haozhong.zhang@intel.com> >> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> > --- >> > drivers/acpi/nfit.c | 7 +++++-- >> > 1 file changed, 5 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c >> > index 0497175ee6cb..008dbaaa2b75 100644 >> > --- a/drivers/acpi/nfit.c >> > +++ b/drivers/acpi/nfit.c >> > @@ -2414,12 +2414,15 @@ static int acpi_nfit_add(struct acpi_device *adev) >> > acpi_desc->nfit = >> > (struct acpi_nfit_header *)obj->buffer.pointer; >> > sz = obj->buffer.length; >> > + rc = acpi_nfit_init(acpi_desc, sz); >> > } else >> > dev_dbg(dev, "%s invalid type %d, ignoring _FIT\n", >> > __func__, (int) obj->type); >> >> 'rc' is not set in this path, so it maybe used uninitialized by 'if (rc)' below. >> Should we set it to a non-zero value in this path? > > 'rc' should be set to 0 here, as what patch 2 does. Sorry for my mistake. No, this is good feedback because patch1 is targeted for -stable. Will fix, thanks! ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-07-15 17:11 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-15 3:28 [PATCH 1/2] nfit: fix _FIT evaluation memory leak Dan Williams 2016-07-15 3:28 ` Dan Williams 2016-07-15 3:29 ` [PATCH 2/2] nfit: cleanup acpi_nfit_init calling convention Dan Williams 2016-07-15 3:29 ` Dan Williams 2016-07-15 8:40 ` joeyli 2016-07-15 8:40 ` joeyli 2016-07-15 5:15 ` [PATCH 1/2] nfit: fix _FIT evaluation memory leak joeyli 2016-07-15 5:15 ` joeyli 2016-07-15 5:15 ` joeyli 2016-07-15 5:27 ` Dan Williams 2016-07-15 5:27 ` Dan Williams 2016-07-15 5:47 ` Xiao Guangrong 2016-07-15 5:47 ` Xiao Guangrong 2016-07-15 14:57 ` Dan Williams 2016-07-15 14:57 ` Dan Williams 2016-07-15 7:55 ` Haozhong Zhang 2016-07-15 7:55 ` Haozhong Zhang 2016-07-15 8:12 ` Haozhong Zhang 2016-07-15 8:12 ` Haozhong Zhang 2016-07-15 17:10 ` Dan Williams 2016-07-15 17:10 ` Dan Williams
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.