| Message ID | 20170403190014.GA20892@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 3, 2017 at 12:00 PM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > On Sat, Apr 01, 2017 at 12:25:20PM -0700, Dan Williams wrote: >> Calls to acpi_get_table() must be paired with acpi_put_table() to undo >> the mapping established by acpi_tb_acquire_table(). >> >> Cc: <stable@vger.kernel.org> >> Fixes: 6b11d1d67713 ("ACPI / osl: Remove acpi_get_table_with_size()/early_acpi_os_unmap_memory() users") >> Cc: Lv Zheng <lv.zheng@intel.com> >> Reported-by: Ross Zwisler <ross.zwisler@linux.intel.com> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/acpi/nfit/core.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c >> index c8ea9d698cd0..6acfea69f061 100644 >> --- a/drivers/acpi/nfit/core.c >> +++ b/drivers/acpi/nfit/core.c >> @@ -2818,6 +2818,11 @@ void acpi_nfit_desc_init(struct acpi_nfit_desc *acpi_desc, struct device *dev) >> } >> EXPORT_SYMBOL_GPL(acpi_nfit_desc_init); >> >> +static void acpi_nfit_put_table(void *table) >> +{ >> + acpi_put_table(table); >> +} >> + >> static int acpi_nfit_add(struct acpi_device *adev) >> { >> struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; >> @@ -2834,6 +2839,10 @@ static int acpi_nfit_add(struct acpi_device *adev) >> dev_dbg(dev, "failed to find NFIT at startup\n"); >> return 0; >> } >> + >> + rc = devm_add_action_or_reset(dev, acpi_nfit_put_table, tbl); >> + if (rc) >> + return rc; >> sz = tbl->length; >> >> acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); > > I've been looking at this this as well, and I think it might be better to just > drop the reference immediately in acpi_nfit_add() after we're done processing > the tables? > > Anyway, here's the patch I was working on, but I think either works. > > ------ 8< ------ > From 4819cd889b08c3c17f8f9fcf895a083f21fa0898 Mon Sep 17 00:00:00 2001 > From: Ross Zwisler <ross.zwisler@linux.intel.com> > Date: Mon, 3 Apr 2017 11:53:32 -0600 > Subject: [PATCH] nfit: release reference on ACPI nfit table > > Currently acpi_nfit_add() grabs a reference to the ACPI NFIT table > structures via acpi_get_table(), but never releases it with > acpi_put_table(). This means that the refcount protecting the ioremap for > the NFIT table never decrements, so the ioremap can never be undone. The > ioremap happens via this path: > > acpi_get_table() > acpi_tb_get_table() > acpi_tb_validate_table() > acpi_tb_acquire_table() > acpi_os_map_memory() > > In practice this fix is correct but won't have a usable visible impact > because the ACPI sysfs code (acpi_table_attr_init() et al.), which > populates /sys/firmware/acpi/tables/NFIT, also keeps a table reference, so > the NFIT table memory will always remain mapped. > > We drop the refcount at the end of acpi_nfit_add() instead of waiting till > driver unload and doing it via acpi_nfit_remove() or something akin to > acpi_nfit_destruct() called via the devm_ interface. This is because > during acpi_nfit_add() we never actually keep any references to the > original ACPI tables. We either copy individual elements out, or we make > whole copies of tables in functions like add_spa(), add memdev(), etc. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > --- > drivers/acpi/nfit/core.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index 662036b..ad0dfd6 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -2833,8 +2833,10 @@ static int acpi_nfit_add(struct acpi_device *adev) > sz = tbl->length; > > acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); > - if (!acpi_desc) > - return -ENOMEM; > + if (!acpi_desc) { > + rc = -ENOMEM; > + goto out; > + } > acpi_nfit_desc_init(acpi_desc, &adev->dev); > > /* Save the acpi header for exporting the revision via sysfs */ > @@ -2857,6 +2859,8 @@ static int acpi_nfit_add(struct acpi_device *adev) > rc = acpi_nfit_init(acpi_desc, (void *) tbl > + sizeof(struct acpi_table_nfit), > sz - sizeof(struct acpi_table_nfit)); > +out: > + acpi_put_table(tbl); > return rc; > } Hi Ross, thanks for this. I'll add your note about this fix not making a difference in practice to my devm_add_action_or_reset() version and drop -stable. I don't want to maintain gotos in the init path if at all possible.
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index 662036b..ad0dfd6 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -2833,8 +2833,10 @@ static int acpi_nfit_add(struct acpi_device *adev) sz = tbl->length; acpi_desc = devm_kzalloc(dev, sizeof(*acpi_desc), GFP_KERNEL); - if (!acpi_desc) - return -ENOMEM; + if (!acpi_desc) { + rc = -ENOMEM; + goto out; + } acpi_nfit_desc_init(acpi_desc, &adev->dev); /* Save the acpi header for exporting the revision via sysfs */ @@ -2857,6 +2859,8 @@ static int acpi_nfit_add(struct acpi_device *adev) rc = acpi_nfit_init(acpi_desc, (void *) tbl + sizeof(struct acpi_table_nfit), sz - sizeof(struct acpi_table_nfit)); +out: + acpi_put_table(tbl); return rc; }