linux-kernel - Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer

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]

Date:   Thu, 28 Jan 2021 00:52:48 -0300
From:   Thiago Jung Bauermann <bauerman@...ux.ibm.com>
To:     Will Deacon <will@...nel.org>
Cc:     Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>,
        zohar@...ux.ibm.com, robh@...nel.org, takahiro.akashi@...aro.org,
        gregkh@...uxfoundation.org, catalin.marinas@....com,
        mpe@...erman.id.au, james.morse@....com, sashal@...nel.org,
        benh@...nel.crashing.org, paulus@...ba.org, frowand.list@...il.com,
        vincenzo.frascino@....com, mark.rutland@....com,
        dmitry.kasatkin@...il.com, jmorris@...ei.org, serge@...lyn.com,
        pasha.tatashin@...een.com, allison@...utok.net,
        masahiroy@...nel.org, bhsharma@...hat.com, mbrugger@...e.com,
        hsinyi@...omium.org, tao.li@...o.com, christophe.leroy@....fr,
        prsriva@...ux.microsoft.com, balajib@...ux.microsoft.com,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v15 09/10] arm64: Call kmalloc() to allocate DTB buffer


Will Deacon <will@...nel.org> writes:

> On Wed, Jan 27, 2021 at 09:59:38AM -0800, Lakshmi Ramasubramanian wrote:
>> On 1/27/21 8:52 AM, Will Deacon wrote:
>> 
>> Hi Will,
>> 
>> > On Fri, Jan 15, 2021 at 09:30:16AM -0800, Lakshmi Ramasubramanian wrote:
>> > > create_dtb() function allocates kernel virtual memory for
>> > > the device tree blob (DTB).  This is not consistent with other
>> > > architectures, such as powerpc, which calls kmalloc() for allocating
>> > > memory for the DTB.
>> > > 
>> > > Call kmalloc() to allocate memory for the DTB, and kfree() to free
>> > > the allocated memory.
>> > > 
>> > > Co-developed-by: Prakhar Srivastava <prsriva@...ux.microsoft.com>
>> > > Signed-off-by: Prakhar Srivastava <prsriva@...ux.microsoft.com>
>> > > Signed-off-by: Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
>> > > ---
>> > >   arch/arm64/kernel/machine_kexec_file.c | 12 +++++++-----
>> > >   1 file changed, 7 insertions(+), 5 deletions(-)
>> > > 
>> > > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
>> > > index 7de9c47dee7c..51c40143d6fa 100644
>> > > --- a/arch/arm64/kernel/machine_kexec_file.c
>> > > +++ b/arch/arm64/kernel/machine_kexec_file.c
>> > > @@ -29,7 +29,7 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
>> > >   int arch_kimage_file_post_load_cleanup(struct kimage *image)
>> > >   {
>> > > -	vfree(image->arch.dtb);
>> > > +	kfree(image->arch.dtb);
>> > >   	image->arch.dtb = NULL;
>> > >   	vfree(image->arch.elf_headers);
>> > > @@ -59,19 +59,21 @@ static int create_dtb(struct kimage *image,
>> > >   			+ cmdline_len + DTB_EXTRA_SPACE;
>> > >   	for (;;) {
>> > > -		buf = vmalloc(buf_size);
>> > > +		buf = kmalloc(buf_size, GFP_KERNEL);
>> > 
>> > Is there a functional need for this patch? I build the 'dtbs' target just
>> > now and sdm845-db845c.dtb is approaching 100K, which feels quite large
>> > for kmalloc().
>> 
>> Changing the allocation from vmalloc() to kmalloc() would help us further
>> consolidate the DTB setup code for powerpc and arm64.
>
> Ok, but at the risk of allocation failure. Can powerpc use vmalloc()
> instead?

I believe this patch stems from this suggestion by Rob Herring:

> This could be taken a step further and do the allocation of the new
> FDT. The difference is arm64 uses vmalloc and powerpc uses kmalloc. The
> arm64 version also retries with a bigger allocation. That seems
> unnecessary.

in https://lore.kernel.org/linux-integrity/20201211221006.1052453-3-robh@kernel.org/

The problem is that this patch implements only part of the suggestion,
which isn't useful in itself. So the patch series should either drop
this patch or consolidate the FDT allocation between the arches.

I just tested on powernv and pseries platforms and powerpc can use
vmalloc for the FDT buffer.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center