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

[Lakshmi Ramasubramanian]

On 1/27/21 8:14 PM, Thiago Jung Bauermann wrote:
> Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> writes:
>
> > On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote:
> > > Will Deacon <will@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >     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@xxxxxxxxxx/
> > > 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.
> >
> > Thanks for verifying on powerpc platform Thiago.
> >
> > I'll update the patch to do the following:
> >
> > => Use vmalloc for FDT buffer allocation on powerpc
> > => Keep vmalloc for arm64, but remove the retry on allocation.
> > => Also, there was a memory leak of FDT buffer in the error code path on arm64,
> > which I'll fix as well.
> >
> > Did I miss anything?
>
> Yes, you missed the second part of Rob's suggestion I was mentioning,
> which is factoring out the code which allocates the new FDT from both
> arm64 and powerpc.

Sure - I'll address that.

thanks,
 -lakshmi