linux-kernel - Re: request_firmware API exhaust memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <r2iac3eb2511004270634q31ad5aa2ne5f6de21355f1a36@mail.gmail.com>
Date:	Tue, 27 Apr 2010 15:34:15 +0200
From:	Kay Sievers <kay.sievers@...y.org>
To:	David Woodhouse <dwmw2@...radead.org>
Cc:	dhowells@...hat.com, Tomas Winkler <tomasw@...il.com>,
	Greg KH <greg@...ah.com>,
	Johannes Berg <johannes@...solutions.net>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Emmanuel Grumbach <egrumbach@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: request_firmware API exhaust memory

On Tue, Apr 27, 2010 at 14:43, David Woodhouse <dwmw2@...radead.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/