[PATCH v3] firmware_class: fix memory leak - free allocated pages

[PATCH v3] firmware_class: fix memory leak - free allocated pages

[Tomas Winkler]

From: David Woodhouse <David.Woodhouse@intel.com>

fix memory leak introduced by the patch 6e03a201bbe:
firmware: speed up request_firmware()

1. vfree won't release pages there were allocated explicitly and mapped
using vmap. The memory has to be vunmap-ed and the pages needs
to be freed explicitly

2. page array is moved into the 'struct
firmware' so that we can free it from release_firmware()
and not only in fw_dev_release()

The fix doesn't break the firmware load speed.

Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Ming Lei <tom.leiming@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Singed-off-by: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: fix authorship of the patch
V3: fix const struct firmware breakage

 drivers/base/firmware_class.c |   26 ++++++++++++++++++++------
 include/linux/firmware.h      |    1 +
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 985da11..4c70b91 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -130,6 +130,17 @@ static ssize_t firmware_loading_show(struct device *dev,
 	return sprintf(buf, "%d\n", loading);
 }
 
+static void firmware_free_data(const struct firmware *fw)
+{
+	int i;
+	vunmap(fw->data);
+	if (fw->pages) {
+		for (i = 0; i < PFN_UP(fw->size); i++)
+			__free_page(fw->pages[i]);
+		kfree(fw->pages);
+	}
+}
+
 /* Some architectures don't have PAGE_KERNEL_RO */
 #ifndef PAGE_KERNEL_RO
 #define PAGE_KERNEL_RO PAGE_KERNEL
@@ -162,21 +173,21 @@ static ssize_t firmware_loading_store(struct device *dev,
 			mutex_unlock(&fw_lock);
 			break;
 		}
-		vfree(fw_priv->fw->data);
-		fw_priv->fw->data = NULL;
+		firmware_free_data(fw_priv->fw);
+		memset(fw_priv->fw, 0, sizeof(struct firmware));
+		/* If the pages are not owned by 'struct firmware' */
 		for (i = 0; i < fw_priv->nr_pages; i++)
 			__free_page(fw_priv->pages[i]);
 		kfree(fw_priv->pages);
 		fw_priv->pages = NULL;
 		fw_priv->page_array_size = 0;
 		fw_priv->nr_pages = 0;
-		fw_priv->fw->size = 0;
 		set_bit(FW_STATUS_LOADING, &fw_priv->status);
 		mutex_unlock(&fw_lock);
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_priv->status)) {
-			vfree(fw_priv->fw->data);
+			vunmap(fw_priv->fw->data);
 			fw_priv->fw->data = vmap(fw_priv->pages,
 						 fw_priv->nr_pages,
 						 0, PAGE_KERNEL_RO);
@@ -184,7 +195,10 @@ static ssize_t firmware_loading_store(struct device *dev,
 				dev_err(dev, "%s: vmap() failed\n", __func__);
 				goto err;
 			}
-			/* Pages will be freed by vfree() */
+			/* Pages are now owned by 'struct firmware' */
+			fw_priv->fw->pages = fw_priv->pages;
+			fw_priv->pages = NULL;
+
 			fw_priv->page_array_size = 0;
 			fw_priv->nr_pages = 0;
 			complete(&fw_priv->completion);
@@ -578,7 +592,7 @@ release_firmware(const struct firmware *fw)
 			if (fw->data == builtin->data)
 				goto free_fw;
 		}
-		vfree(fw->data);
+		firmware_free_data(fw);
 	free_fw:
 		kfree(fw);
 	}
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 043811f..53d1e6c 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -12,6 +12,7 @@
 struct firmware {
 	size_t size;
 	const u8 *data;
+	struct page **pages;
 };
 
 struct device;
-- 
1.6.6.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages

[Greg KH]

On Sun, May 02, 2010 at 11:21:21AM +0300, Tomas Winkler wrote:
> From: David Woodhouse <David.Woodhouse@intel.com>
> 
> fix memory leak introduced by the patch 6e03a201bbe:
> firmware: speed up request_firmware()
> 
> 1. vfree won't release pages there were allocated explicitly and mapped
> using vmap. The memory has to be vunmap-ed and the pages needs
> to be freed explicitly
> 
> 2. page array is moved into the 'struct
> firmware' so that we can free it from release_firmware()
> and not only in fw_dev_release()
> 
> The fix doesn't break the firmware load speed.
> 
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Ming Lei <tom.leiming@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Singed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2: fix authorship of the patch
> V3: fix const struct firmware breakage

Thanks, this looks much better, I've applied it now.

greg k-h

Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages

[Pavan Savoy]

--- On Mon, 3/5/10, Greg KH <greg@kroah.com> wrote:

> From: Greg KH <greg@kroah.com>
> Subject: Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages
> To: "Tomas Winkler" <tomas.winkler@intel.com>
> Cc: "Greg Kroah-Hartman" <gregkh@suse.de>, linux-kernel@vger.kernel.org, "David Woodhouse" <dwmw2@infradead.org>, "Kay Sievers" <kay.sievers@vrfy.org>, "David Woodhouse" <David.Woodhouse@intel.com>, "Johannes Berg" <johannes@sipsolutions.net>, "Ming Lei" <tom.leiming@gmail.com>, "Catalin Marinas" <catalin.marinas@arm.com>
> Date: Monday, 3 May, 2010, 10:34 PM
> On Sun, May 02, 2010 at 11:21:21AM
> +0300, Tomas Winkler wrote:
> > From: David Woodhouse <David.Woodhouse@intel.com>
> > 
> > fix memory leak introduced by the patch 6e03a201bbe:
> > firmware: speed up request_firmware()
> > 
> > 1. vfree won't release pages there were allocated
> explicitly and mapped
> > using vmap. The memory has to be vunmap-ed and the
> pages needs
> > to be freed explicitly
> > 
> > 2. page array is moved into the 'struct
> > firmware' so that we can free it from
> release_firmware()
> > and not only in fw_dev_release()
> > 
> > The fix doesn't break the firmware load speed.
> > 
> > Cc: Johannes Berg <johannes@sipsolutions.net>
> > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > Cc: Ming Lei <tom.leiming@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Singed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > V2: fix authorship of the patch
> > V3: fix const struct firmware breakage
> 
> Thanks, this looks much better, I've applied it now.

Sorry to bump-in the middle.
But as a user of firmware class, what should now my fw_entry structure be?
const struct firmware *fw_entry or,
struct firmware *fw_entry - without the const ..


> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages

[Greg KH]

On Tue, May 04, 2010 at 12:16:44AM +0530, Pavan Savoy wrote:
> 
> 
> --- On Mon, 3/5/10, Greg KH <greg@kroah.com> wrote:
> 
> > From: Greg KH <greg@kroah.com>
> > Subject: Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages
> > To: "Tomas Winkler" <tomas.winkler@intel.com>
> > Cc: "Greg Kroah-Hartman" <gregkh@suse.de>, linux-kernel@vger.kernel.org, "David Woodhouse" <dwmw2@infradead.org>, "Kay Sievers" <kay.sievers@vrfy.org>, "David Woodhouse" <David.Woodhouse@intel.com>, "Johannes Berg" <johannes@sipsolutions.net>, "Ming Lei" <tom.leiming@gmail.com>, "Catalin Marinas" <catalin.marinas@arm.com>
> > Date: Monday, 3 May, 2010, 10:34 PM
> > On Sun, May 02, 2010 at 11:21:21AM
> > +0300, Tomas Winkler wrote:
> > > From: David Woodhouse <David.Woodhouse@intel.com>
> > > 
> > > fix memory leak introduced by the patch 6e03a201bbe:
> > > firmware: speed up request_firmware()
> > > 
> > > 1. vfree won't release pages there were allocated
> > explicitly and mapped
> > > using vmap. The memory has to be vunmap-ed and the
> > pages needs
> > > to be freed explicitly
> > > 
> > > 2. page array is moved into the 'struct
> > > firmware' so that we can free it from
> > release_firmware()
> > > and not only in fw_dev_release()
> > > 
> > > The fix doesn't break the firmware load speed.
> > > 
> > > Cc: Johannes Berg <johannes@sipsolutions.net>
> > > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > > Cc: Ming Lei <tom.leiming@gmail.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Singed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > > V2: fix authorship of the patch
> > > V3: fix const struct firmware breakage
> > 
> > Thanks, this looks much better, I've applied it now.
> 
> Sorry to bump-in the middle.
> But as a user of firmware class, what should now my fw_entry structure be?
> const struct firmware *fw_entry or,
> struct firmware *fw_entry - without the const ..

You should leave it to be whatever it was before this patch, it has not
changed anything now.

thanks,

greg k-h

Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages

[Pavan Savoy]

--- On Tue, 4/5/10, Greg KH <greg@kroah.com> wrote:

> From: Greg KH <greg@kroah.com>
> Subject: Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages
> To: "Pavan Savoy" <pavan_savoy@yahoo.co.in>
> Cc: "Tomas Winkler" <tomas.winkler@intel.com>, linux-kernel@vger.kernel.org
> Date: Tuesday, 4 May, 2010, 7:33 PM
> On Tue, May 04, 2010 at 12:16:44AM
> +0530, Pavan Savoy wrote:
> > 
> > 
> > --- On Mon, 3/5/10, Greg KH <greg@kroah.com>
> wrote:
> > 
> > > From: Greg KH <greg@kroah.com>
> > > Subject: Re: [PATCH v3] firmware_class: fix
> memory leak - free allocated pages
> > > To: "Tomas Winkler" <tomas.winkler@intel.com>
> > > Cc: "Greg Kroah-Hartman" <gregkh@suse.de>,
> linux-kernel@vger.kernel.org,
> "David Woodhouse" <dwmw2@infradead.org>,
> "Kay Sievers" <kay.sievers@vrfy.org>,
> "David Woodhouse" <David.Woodhouse@intel.com>,
> "Johannes Berg" <johannes@sipsolutions.net>,
> "Ming Lei" <tom.leiming@gmail.com>,
> "Catalin Marinas" <catalin.marinas@arm.com>
> > > Date: Monday, 3 May, 2010, 10:34 PM
> > > On Sun, May 02, 2010 at 11:21:21AM
> > > +0300, Tomas Winkler wrote:
> > > > From: David Woodhouse <David.Woodhouse@intel.com>
> > > > 
> > > > fix memory leak introduced by the patch
> 6e03a201bbe:
> > > > firmware: speed up request_firmware()
> > > > 
> > > > 1. vfree won't release pages there were
> allocated
> > > explicitly and mapped
> > > > using vmap. The memory has to be vunmap-ed
> and the
> > > pages needs
> > > > to be freed explicitly
> > > > 
> > > > 2. page array is moved into the 'struct
> > > > firmware' so that we can free it from
> > > release_firmware()
> > > > and not only in fw_dev_release()
> > > > 
> > > > The fix doesn't break the firmware load
> speed.
> > > > 
> > > > Cc: Johannes Berg <johannes@sipsolutions.net>
> > > > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > > > Cc: Ming Lei <tom.leiming@gmail.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Singed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> > > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > ---
> > > > V2: fix authorship of the patch
> > > > V3: fix const struct firmware breakage
> > > 
> > > Thanks, this looks much better, I've applied it
> now.
> > 
> > Sorry to bump-in the middle.
> > But as a user of firmware class, what should now my
> fw_entry structure be?
> > const struct firmware *fw_entry or,
> > struct firmware *fw_entry - without the const ..
> 
> You should leave it to be whatever it was before this
> patch, it has not
> changed anything now.

Ok, However this patch has changed the release_firmware,
-release_firmware(const struct firmware *fw)
+release_firmware(struct firmware *fw)


> thanks,
> 
> greg k-h
> 

Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages

[Greg KH]

On Tue, May 04, 2010 at 08:07:33PM +0530, Pavan Savoy wrote:
> 
> --- On Tue, 4/5/10, Greg KH <greg@kroah.com> wrote:
> 
> > From: Greg KH <greg@kroah.com>
> > Subject: Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages
> > To: "Pavan Savoy" <pavan_savoy@yahoo.co.in>
> > Cc: "Tomas Winkler" <tomas.winkler@intel.com>, linux-kernel@vger.kernel.org
> > Date: Tuesday, 4 May, 2010, 7:33 PM
> > On Tue, May 04, 2010 at 12:16:44AM
> > +0530, Pavan Savoy wrote:
> > > 
> > > 
> > > --- On Mon, 3/5/10, Greg KH <greg@kroah.com>
> > wrote:
> > > 
> > > > From: Greg KH <greg@kroah.com>
> > > > Subject: Re: [PATCH v3] firmware_class: fix
> > memory leak - free allocated pages
> > > > To: "Tomas Winkler" <tomas.winkler@intel.com>
> > > > Cc: "Greg Kroah-Hartman" <gregkh@suse.de>,
> > linux-kernel@vger.kernel.org,
> > "David Woodhouse" <dwmw2@infradead.org>,
> > "Kay Sievers" <kay.sievers@vrfy.org>,
> > "David Woodhouse" <David.Woodhouse@intel.com>,
> > "Johannes Berg" <johannes@sipsolutions.net>,
> > "Ming Lei" <tom.leiming@gmail.com>,
> > "Catalin Marinas" <catalin.marinas@arm.com>
> > > > Date: Monday, 3 May, 2010, 10:34 PM
> > > > On Sun, May 02, 2010 at 11:21:21AM
> > > > +0300, Tomas Winkler wrote:
> > > > > From: David Woodhouse <David.Woodhouse@intel.com>
> > > > > 
> > > > > fix memory leak introduced by the patch
> > 6e03a201bbe:
> > > > > firmware: speed up request_firmware()
> > > > > 
> > > > > 1. vfree won't release pages there were
> > allocated
> > > > explicitly and mapped
> > > > > using vmap. The memory has to be vunmap-ed
> > and the
> > > > pages needs
> > > > > to be freed explicitly
> > > > > 
> > > > > 2. page array is moved into the 'struct
> > > > > firmware' so that we can free it from
> > > > release_firmware()
> > > > > and not only in fw_dev_release()
> > > > > 
> > > > > The fix doesn't break the firmware load
> > speed.
> > > > > 
> > > > > Cc: Johannes Berg <johannes@sipsolutions.net>
> > > > > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > > > > Cc: Ming Lei <tom.leiming@gmail.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Singed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> > > > > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > > ---
> > > > > V2: fix authorship of the patch
> > > > > V3: fix const struct firmware breakage
> > > > 
> > > > Thanks, this looks much better, I've applied it
> > now.
> > > 
> > > Sorry to bump-in the middle.
> > > But as a user of firmware class, what should now my
> > fw_entry structure be?
> > > const struct firmware *fw_entry or,
> > > struct firmware *fw_entry - without the const ..
> > 
> > You should leave it to be whatever it was before this
> > patch, it has not
> > changed anything now.
> 
> Ok, However this patch has changed the release_firmware,
> -release_firmware(const struct firmware *fw)
> +release_firmware(struct firmware *fw)

Does your code now complain about a warning?  If so, do you have a
pointer to it?

thanks,

greg k-h

Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages

[Pavan Savoy]

--- On Tue, 4/5/10, Greg KH <greg@kroah.com> wrote:

> From: Greg KH <greg@kroah.com>
> Subject: Re: [PATCH v3] firmware_class: fix memory leak - free allocated pages
> To: "Pavan Savoy" <pavan_savoy@yahoo.co.in>
> Cc: "Tomas Winkler" <tomas.winkler@intel.com>, linux-kernel@vger.kernel.org
> Date: Tuesday, 4 May, 2010, 9:17 PM
> On Tue, May 04, 2010 at 08:07:33PM
> +0530, Pavan Savoy wrote:
> > 
> > --- On Tue, 4/5/10, Greg KH <greg@kroah.com>
> wrote:
> > 
> > > From: Greg KH <greg@kroah.com>
> > > Subject: Re: [PATCH v3] firmware_class: fix
> memory leak - free allocated pages
> > > To: "Pavan Savoy" <pavan_savoy@yahoo.co.in>
> > > Cc: "Tomas Winkler" <tomas.winkler@intel.com>,
> linux-kernel@vger.kernel.org
> > > Date: Tuesday, 4 May, 2010, 7:33 PM
> > > On Tue, May 04, 2010 at 12:16:44AM
> > > +0530, Pavan Savoy wrote:
> > > > 
> > > > 
> > > > --- On Mon, 3/5/10, Greg KH <greg@kroah.com>
> > > wrote:
> > > > 
> > > > > From: Greg KH <greg@kroah.com>
> > > > > Subject: Re: [PATCH v3] firmware_class:
> fix
> > > memory leak - free allocated pages
> > > > > To: "Tomas Winkler" <tomas.winkler@intel.com>
> > > > > Cc: "Greg Kroah-Hartman" <gregkh@suse.de>,
> > > linux-kernel@vger.kernel.org,
> > > "David Woodhouse" <dwmw2@infradead.org>,
> > > "Kay Sievers" <kay.sievers@vrfy.org>,
> > > "David Woodhouse" <David.Woodhouse@intel.com>,
> > > "Johannes Berg" <johannes@sipsolutions.net>,
> > > "Ming Lei" <tom.leiming@gmail.com>,
> > > "Catalin Marinas" <catalin.marinas@arm.com>
> > > > > Date: Monday, 3 May, 2010, 10:34 PM
> > > > > On Sun, May 02, 2010 at 11:21:21AM
> > > > > +0300, Tomas Winkler wrote:
> > > > > > From: David Woodhouse <David.Woodhouse@intel.com>
> > > > > > 
> > > > > > fix memory leak introduced by the
> patch
> > > 6e03a201bbe:
> > > > > > firmware: speed up
> request_firmware()
> > > > > > 
> > > > > > 1. vfree won't release pages there
> were
> > > allocated
> > > > > explicitly and mapped
> > > > > > using vmap. The memory has to be
> vunmap-ed
> > > and the
> > > > > pages needs
> > > > > > to be freed explicitly
> > > > > > 
> > > > > > 2. page array is moved into the
> 'struct
> > > > > > firmware' so that we can free it
> from
> > > > > release_firmware()
> > > > > > and not only in fw_dev_release()
> > > > > > 
> > > > > > The fix doesn't break the firmware
> load
> > > speed.
> > > > > > 
> > > > > > Cc: Johannes Berg <johannes@sipsolutions.net>
> > > > > > Cc: Greg Kroah-Hartman <gregkh@suse.de>
> > > > > > Cc: Ming Lei <tom.leiming@gmail.com>
> > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > Singed-off-by: Kay Sievers <kay.sievers@vrfy.org>
> > > > > > Signed-off-by: David Woodhouse
> <David.Woodhouse@intel.com>
> > > > > > Signed-off-by: Tomas Winkler
> <tomas.winkler@intel.com>
> > > > > > ---
> > > > > > V2: fix authorship of the patch
> > > > > > V3: fix const struct firmware
> breakage
> > > > > 
> > > > > Thanks, this looks much better, I've
> applied it
> > > now.
> > > > 
> > > > Sorry to bump-in the middle.
> > > > But as a user of firmware class, what should
> now my
> > > fw_entry structure be?
> > > > const struct firmware *fw_entry or,
> > > > struct firmware *fw_entry - without the
> const ..
> > > 
> > > You should leave it to be whatever it was before
> this
> > > patch, it has not
> > > changed anything now.
> > 
> > Ok, However this patch has changed the
> release_firmware,
> > -release_firmware(const struct firmware *fw)
> > +release_firmware(struct firmware *fw)
> 
> Does your code now complain about a warning?  If so,
> do you have a
> pointer to it?

Yes, code now has warnings, I have something like,
        const struct firmware *fw_entry; declared in my header file,
and use it as,
            request_firmware(&kim_gdata->fw_entry, bts_scr_name,
                             &kim_gdata->kim_pdev->dev);
and then I get warnings at,
        release_firmware(kim_gdata->fw_entry);

You want me to have a pointer to &kw_gdata->fw_entry which is const ?
as in 
const struct firmware **fw_const_ptr;
fw_const_ptr = &kim_gdata->fw_entry; ?


> thanks,
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>