request_firmware API exhaust memory
in [Kernel]
|
Prev: uml: i386: Avoid redefinition of NR_syscalls
Next: rcu: slim down rcutiny by removing rcu_scheduler_active and friends
From: David Woodhouse on 27 Apr 2010 08:50 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kay Sievers on 27 Apr 2010 09:40 On Tue, Apr 27, 2010 at 14:43, David Woodhouse <dwmw2(a)infradead.org> wrote: > 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. Yeah. I guess the problem with the old code before you made if faster was that it was doing vmalloc()->memcpy()->vfree() in a loop. A vrealloc(), which we don't have today, could be made reasonable fast, I guess. > This untested patch attempts to put the page array into the 'struct > firmware' so that we can free it from release_firmware(). Looks good. Seems to work without problems and without leaking memory. Misses only the member in the struct firmware though. :) > 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. Yeah, we could just do that in a firmware_get_data() accessor function and call vmap() there on-demand. That way we would just account and copy pages in the firmware class until the data is actually accessed. Existing users would just need to change the direct ->data access to firmware_get_data(). Thanks, Kay -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kay Sievers on 28 Apr 2010 04:30 On Tue, Apr 27, 2010 at 15:34, Kay Sievers <kay.sievers(a)vrfy.org> wrote: > On Tue, Apr 27, 2010 at 14:43, David Woodhouse <dwmw2(a)infradead.org> wrote: >> This untested patch attempts to put the page array into the 'struct >> firmware' so that we can free it from release_firmware(). > > Looks good. Seems to work without problems and without leaking memory. > > Misses only the member in the struct firmware though. :) Thomas, any chance to test David's patch, if that solves the issues you've seen? Just add the missing line: --- 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; }; Thanks, Kay -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tomas Winkler on 28 Apr 2010 09:10
On Wed, Apr 28, 2010 at 11:23 AM, Kay Sievers <kay.sievers(a)vrfy.org> wrote: > On Tue, Apr 27, 2010 at 15:34, Kay Sievers <kay.sievers(a)vrfy.org> wrote: >> On Tue, Apr 27, 2010 at 14:43, David Woodhouse <dwmw2(a)infradead.org> wrote: >>> This untested patch attempts to put the page array into the 'struct >>> firmware' so that we can free it from release_firmware(). >> >> Looks good. Seems to work without problems and without leaking memory. >> >> Misses only the member in the struct firmware though. :) > > Thomas, any chance to test David's patch, if that solves the issues you've seen? > > Just add the missing line: > > --- 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; > }; > > In progress. Will post results later today (timezone:GMT+2) Thanks Tomas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |