From patchwork Mon Apr 3 19:00:14 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ross Zwisler X-Patchwork-Id: 9660355 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A35FF6032D for ; Mon, 3 Apr 2017 19:00:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 962ED284C2 for ; Mon, 3 Apr 2017 19:00:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8951D284F5; Mon, 3 Apr 2017 19:00:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 Received: from ml01.01.org (ml01.01.org [198.145.21.10]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 396F7284C2 for ; Mon, 3 Apr 2017 19:00:15 +0000 (UTC) Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id BB974219392F9; Mon, 3 Apr 2017 12:00:15 -0700 (PDT) X-Original-To: linux-nvdimm@lists.01.org Delivered-To: linux-nvdimm@lists.01.org Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C6391219392E4 for ; Mon, 3 Apr 2017 12:00:14 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP; 03 Apr 2017 12:00:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,271,1486454400"; d="scan'208";a="841350080" Received: from theros.lm.intel.com (HELO linux.intel.com) ([10.232.112.77]) by FMSMGA003.fm.intel.com with ESMTP; 03 Apr 2017 12:00:14 -0700 Date: Mon, 3 Apr 2017 13:00:14 -0600 From: Ross Zwisler To: Dan Williams Subject: Re: [PATCH] acpi, nfit: fix acpi_get_table leak Message-ID: <20170403190014.GA20892@linux.intel.com> References: <149107472042.16205.8935477753191459036.stgit@dwillia2-desk3.amr.corp.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <149107472042.16205.8935477753191459036.stgit@dwillia2-desk3.amr.corp.intel.com> User-Agent: Mutt/1.8.0 (2017-02-23) X-BeenThere: linux-nvdimm@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "Linux-nvdimm developer list." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-acpi@vger.kernel.org, Lv Zheng , stable@vger.kernel.org, linux-nvdimm@lists.01.org Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" X-Virus-Scanned: ClamAV using ClamSMTP 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: > Fixes: 6b11d1d67713 ("ACPI / osl: Remove acpi_get_table_with_size()/early_acpi_os_unmap_memory() users") > Cc: Lv Zheng > Reported-by: Ross Zwisler > Signed-off-by: Dan Williams > --- > 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 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 --- 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; }