[v15,09/10] arm64: Call kmalloc() to allocate DTB buffer
diff mbox series

Message ID 20210115173017.30617-10-nramas@linux.microsoft.com
State New, archived
Headers show
Series
  • Carry forward IMA measurement log on kexec on ARM64
Related show

Commit Message

Lakshmi Ramasubramanian Jan. 15, 2021, 5:30 p.m. UTC
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@linux.microsoft.com>
Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 arch/arm64/kernel/machine_kexec_file.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Mimi Zohar Jan. 17, 2021, 1:23 p.m. UTC | #1
Hi Ard,

On Fri, 2021-01-15 at 09:30 -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.

The vmalloc() function description says, "vmalloc - allocate virtually
contiguous memory".  I'd appreciate your reviewing this patch, in
particular, which replaces vmalloc() with kmalloc().

thanks,

Mimi

> 
> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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);
>  		if (!buf)
>  			return -ENOMEM;
>  
>  		/* duplicate a device tree blob */
>  		ret = fdt_open_into(initial_boot_params, buf, buf_size);
> -		if (ret)
> +		if (ret) {
> +			kfree(buf);
>  			return -EINVAL;
> +		}
>  
>  		ret = of_kexec_setup_new_fdt(image, buf, initrd_load_addr,
>  					     initrd_len, cmdline);
>  		if (ret) {
> -			vfree(buf);
> +			kfree(buf);
>  			if (ret == -ENOMEM) {
>  				/* unlikely, but just in case */
>  				buf_size += DTB_EXTRA_SPACE;
> @@ -217,6 +219,6 @@ int load_other_segments(struct kimage *image,
>  	return 0;
>  
>  out_err:
> -	vfree(dtb);
> +	kfree(dtb);
>  	return ret;
>  }
Will Deacon Jan. 27, 2021, 4:52 p.m. UTC | #2
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@linux.microsoft.com>
> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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().

Will
Lakshmi Ramasubramanian Jan. 27, 2021, 5:59 p.m. UTC | #3
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@linux.microsoft.com>
>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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.

thanks,
  -lakshmi
Will Deacon Jan. 27, 2021, 6:43 p.m. UTC | #4
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@linux.microsoft.com>
> > > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
> > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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?

Will
Thiago Jung Bauermann Jan. 28, 2021, 3:52 a.m. UTC | #5
Will Deacon <will@kernel.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@linux.microsoft.com>
>> > > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
>> > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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.
Lakshmi Ramasubramanian Jan. 28, 2021, 4 a.m. UTC | #6
On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote:
> 
> Will Deacon <will@kernel.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@linux.microsoft.com>
>>>>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
>>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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.
> 

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?

thanks,
  -lakshmi
Thiago Jung Bauermann Jan. 28, 2021, 4:14 a.m. UTC | #7
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:

> On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote:
>> Will Deacon <will@kernel.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@linux.microsoft.com>
>>>>>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
>>>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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.
>> 
>
> 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.
Lakshmi Ramasubramanian Jan. 28, 2021, 4:33 a.m. UTC | #8
On 1/27/21 8:14 PM, Thiago Jung Bauermann wrote:
> 
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> 
>> On 1/27/21 7:52 PM, Thiago Jung Bauermann wrote:
>>> Will Deacon <will@kernel.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@linux.microsoft.com>
>>>>>>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com>
>>>>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.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.
>>>
>>
>> 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
Joe Perches Jan. 28, 2021, 4:57 a.m. UTC | #9
On Thu, 2021-01-28 at 00:52 -0300, Thiago Jung Bauermann wrote:
> 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.

Perhaps more sensible to use kvmalloc/kvfree.
Thiago Jung Bauermann Feb. 1, 2021, 6:58 p.m. UTC | #10
Joe Perches <joe@perches.com> writes:

> On Thu, 2021-01-28 at 00:52 -0300, Thiago Jung Bauermann wrote:
>> 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.
>
> Perhaps more sensible to use kvmalloc/kvfree.

That's true. Converting both arm64 to powerpc to kvmalloc/kvfree is a
good option. I don't think it's that much better though, because
kexec_file_load() is called infrequently and doesn't need to be fast so
the vmalloc() overhead isn't important in practice.

Patch
diff mbox series

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);
 		if (!buf)
 			return -ENOMEM;
 
 		/* duplicate a device tree blob */
 		ret = fdt_open_into(initial_boot_params, buf, buf_size);
-		if (ret)
+		if (ret) {
+			kfree(buf);
 			return -EINVAL;
+		}
 
 		ret = of_kexec_setup_new_fdt(image, buf, initrd_load_addr,
 					     initrd_len, cmdline);
 		if (ret) {
-			vfree(buf);
+			kfree(buf);
 			if (ret == -ENOMEM) {
 				/* unlikely, but just in case */
 				buf_size += DTB_EXTRA_SPACE;
@@ -217,6 +219,6 @@  int load_other_segments(struct kimage *image,
 	return 0;
 
 out_err:
-	vfree(dtb);
+	kfree(dtb);
 	return ret;
 }