Re: request_firmware API exhaust memory

[David Woodhouse <dwmw2@infradead.org>]

On Tue, 2010-04-27 at 14:05 +0200, Kay Sievers wrote:
> 
> The patch I posted makes the issue go away. It's still not the right
> fix, because the pages are only get freed when the device id cleaned
> up, not on calling release_firmware. But it should illustrate the
> underlying issue, and that there is no leaked memory anymore.
> 
> >  I think this needs some more review.
> 
> If David does not fix it, it probably just needs to be reverted. And
> instead of implementing our own "memory management", we should rather
> add a vrealloc(), and the firmware loader should use that. 

The whole point was to avoid the vrealloc(). We really don't want to be
screwing with page tables, globally, for each page written from
userspace.

This untested patch attempts to put the page array into the 'struct
firmware' so that we can free it from release_firmware().

It would actually be nice if we could make that the _primary_ method of
returning data to drivers, and we could ditch the vmap() requirement
altogether... drivers which really need it to be virtually contiguous
can depend on CONFIG_MMU and do the vmap() for themselves.

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 985da11..cc9a79b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -162,8 +162,13 @@ static ssize_t firmware_loading_store(struct device *dev,
 			mutex_unlock(&fw_lock);
 			break;
 		}
-		vfree(fw_priv->fw->data);
+		vunmap(fw_priv->fw->data);
 		fw_priv->fw->data = NULL;
+		if (fw_priv->fw->pages) {
+			for (i = 0; i < PFN_UP(fw_priv->fw->size); i++)
+				__free_page(fw_priv->fw->pages[i]);
+			kfree(fw_priv->fw->pages);
+		}
 		for (i = 0; i < fw_priv->nr_pages; i++)
 			__free_page(fw_priv->pages[i]);
 		kfree(fw_priv->pages);
@@ -176,7 +181,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 		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 +189,9 @@ 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);
@@ -571,6 +578,7 @@ void
 release_firmware(const struct firmware *fw)
 {
 	struct builtin_fw *builtin;
+	int i;
 
 	if (fw) {
 		for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
@@ -578,7 +586,12 @@ release_firmware(const struct firmware *fw)
 			if (fw->data == builtin->data)
 				goto free_fw;
 		}
-		vfree(fw->data);
+		vunmap(fw->data);
+		if (fw->pages) {
+			for (i = 0; i < PFN_UP(fw->size); i++)
+				__free_page(fw->pages[i]);
+			kfree(fw->pages);
+		}
 	free_fw:
 		kfree(fw);
 	}


-- 
dwmw2