From: Sai Praneeth <[email protected]>
efi_memmap_install(), unmaps the existing memory map and installs the
new memory map but doesn't free the memory allocated to the existing
memory map. Fortunately, the details about the existing memory map are
stored in efi.memmap. Hence, use them to free the memory.
Signed-off-by: Sai Praneeth Prakhya <[email protected]>
Reported-by: Ard Biesheuvel <[email protected]>
Cc: Lee Chun-Yi <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Young <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Cc: Bhupesh Sharma <[email protected]>
Cc: Ricardo Neri <[email protected]>
Cc: Ravi Shankar <[email protected]>
Cc: Matt Fleming <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
---
Note: Patch based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
drivers/firmware/efi/memmap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
index 678e85704054..68b27b14fe94 100644
--- a/drivers/firmware/efi/memmap.c
+++ b/drivers/firmware/efi/memmap.c
@@ -229,6 +229,9 @@ int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
efi_memmap_unmap();
+ /* Free the memory allocated to the existing memory map */
+ efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
+
data.phys_map = addr;
data.size = efi.memmap.desc_size * nr_map;
data.desc_version = efi.memmap.desc_version;
--
2.7.4
Hi Sai,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sai-Praneeth-Prakhya/efi-Free-existing-memory-map-before-installing-new-memory-map/20180626-104301
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/firmware//efi/memmap.c: In function 'efi_memmap_install':
>> drivers/firmware//efi/memmap.c:199:2: error: implicit declaration of function 'efi_memmap_free'; did you mean 'vmemmap_free'? [-Werror=implicit-function-declaration]
efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
^~~~~~~~~~~~~~~
vmemmap_free
cc1: some warnings being treated as errors
vim +199 drivers/firmware//efi/memmap.c
180
181 /**
182 * efi_memmap_install - Install a new EFI memory map in efi.memmap
183 * @addr: Physical address of the memory map
184 * @nr_map: Number of entries in the memory map
185 *
186 * Unlike efi_memmap_init_*(), this function does not allow the caller
187 * to switch from early to late mappings. It simply uses the existing
188 * mapping function and installs the new memmap.
189 *
190 * Returns zero on success, a negative error code on failure.
191 */
192 int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
193 {
194 struct efi_memory_map_data data;
195
196 efi_memmap_unmap();
197
198 /* Free the memory allocated to the existing memory map */
> 199 efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
200
201 data.phys_map = addr;
202 data.size = efi.memmap.desc_size * nr_map;
203 data.desc_version = efi.memmap.desc_version;
204 data.desc_size = efi.memmap.desc_size;
205
206 return __efi_memmap_init(&data, efi.memmap.late);
207 }
208
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Sai,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Sai-Praneeth-Prakhya/efi-Free-existing-memory-map-before-installing-new-memory-map/20180626-104301
config: x86_64-randconfig-x003-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/firmware/efi/memmap.c: In function 'efi_memmap_install':
>> drivers/firmware/efi/memmap.c:199:2: error: implicit declaration of function 'efi_memmap_free'; did you mean 'efi_memmap_walk'? [-Werror=implicit-function-declaration]
efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
^~~~~~~~~~~~~~~
efi_memmap_walk
cc1: some warnings being treated as errors
vim +199 drivers/firmware/efi/memmap.c
180
181 /**
182 * efi_memmap_install - Install a new EFI memory map in efi.memmap
183 * @addr: Physical address of the memory map
184 * @nr_map: Number of entries in the memory map
185 *
186 * Unlike efi_memmap_init_*(), this function does not allow the caller
187 * to switch from early to late mappings. It simply uses the existing
188 * mapping function and installs the new memmap.
189 *
190 * Returns zero on success, a negative error code on failure.
191 */
192 int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
193 {
194 struct efi_memory_map_data data;
195
196 efi_memmap_unmap();
197
198 /* Free the memory allocated to the existing memory map */
> 199 efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
200
201 data.phys_map = addr;
202 data.size = efi.memmap.desc_size * nr_map;
203 data.desc_version = efi.memmap.desc_version;
204 data.desc_size = efi.memmap.desc_size;
205
206 return __efi_memmap_init(&data, efi.memmap.late);
207 }
208
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
> Hi Sai,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
> the wrong git tree, please drop us a note to help improve the system]
Since efi_memmap_free() is a recently introduced API [1], please note that the patch
is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
[1] https://lkml.org/lkml/2018/6/22/39
Regards,
Sai
On 26 June 2018 at 04:41, Sai Praneeth Prakhya
<[email protected]> wrote:
> From: Sai Praneeth <[email protected]>
>
> efi_memmap_install(), unmaps the existing memory map and installs the
> new memory map but doesn't free the memory allocated to the existing
> memory map. Fortunately, the details about the existing memory map are
> stored in efi.memmap. Hence, use them to free the memory.
>
> Signed-off-by: Sai Praneeth Prakhya <[email protected]>
> Reported-by: Ard Biesheuvel <[email protected]>
> Cc: Lee Chun-Yi <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Dave Young <[email protected]>
> Cc: Laszlo Ersek <[email protected]>
> Cc: Bhupesh Sharma <[email protected]>
> Cc: Ricardo Neri <[email protected]>
> Cc: Ravi Shankar <[email protected]>
> Cc: Matt Fleming <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> ---
>
> Note: Patch based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>
> drivers/firmware/efi/memmap.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 678e85704054..68b27b14fe94 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -229,6 +229,9 @@ int __init efi_memmap_install(phys_addr_t addr, unsigned int nr_map)
>
> efi_memmap_unmap();
>
> + /* Free the memory allocated to the existing memory map */
> + efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
> +
> data.phys_map = addr;
> data.size = efi.memmap.desc_size * nr_map;
> data.desc_version = efi.memmap.desc_version;
> --
> 2.7.4
>
If only it were so simple :-)
At this point, efi.memmap.phys_map could point to memory that was
allocated early, allocated late or simply passed to the OS at boot
time by the stub (in which case it was memblock_reserve()d but not
memblock_alloc()d, and it should not be freed)
So only allocations made with efi_memmap_alloc() should be freed here.
I'm not sure /how/ we should keep track of that: perhaps it is simply
a matter of replacing the boolean 'late' with an enum that describes
where the memory came from that phys_map points to.
> > + /* Free the memory allocated to the existing memory map */
> > + efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map,
> > + efi.memmap.late);
> > +
> > data.phys_map = addr;
> > data.size = efi.memmap.desc_size * nr_map;
> > data.desc_version = efi.memmap.desc_version;
> > --
> > 2.7.4
> >
>
> If only it were so simple :-)
>
> At this point, efi.memmap.phys_map could point to memory that was allocated
> early, allocated late or simply passed to the OS at boot time by the stub (in
> which case it was memblock_reserve()d but not memblock_alloc()d, and it
> should not be freed)
>
Yes, completely agree that there could be three types of allocations for memmap.
I thought,
efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
should work because the previous type of allocation should have been recorded in efi.memmap.late.
But, now I see this will fail for memblock_reserved() memory because it will be mistaken to
memblock_alloced() (I assumed both are almost similar :().
> So only allocations made with efi_memmap_alloc() should be freed here.
Makes sense and I think that also means efi_memmap_free() should be called from function
that called efi_memmap_alloc() and not efi_memmap_install().
> I'm not sure /how/ we should keep track of that: perhaps it is simply a matter of
> replacing the boolean 'late' with an enum that describes where the memory
> came from that phys_map points to.
I did try changing boolean late to enum and it seems to be working fine. I will do more
testing/clean up and will submit a patch for review.
Also, could you please clarify if there is any specific reason why memory allocated
using memblock_reserve() shouldn't be freed. I mean, not with memblock_free() but I
think we could make it _available_ using free_bootmem() (or something similar, please
correct me if this is not the right API). If we allocate and install a new memory map (as
in case with efi_fake_memmap()), I think we should free the memory used by memory map
originally passed by EFI stub, because, at any point of time there should only be one active
memory map. If we don't free the original memory map passed by EFI stub, we will be having
two and hence will be leaking memory.
Regards,
Sai
Hi,
On 06/26, Prakhya, Sai Praneeth wrote:
>>
>> Hi Sai,
>>
>> Thank you for the patch! Yet something to improve:
>>
>> [auto build test ERROR on linus/master]
>> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
>> the wrong git tree, please drop us a note to help improve the system]
>
>Since efi_memmap_free() is a recently introduced API [1], please note that the patch
>is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
I noticed the official efi tree recored in MAINTAINERS is git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
are they identical?
EXTENSIBLE FIRMWARE INTERFACE (EFI)
M: Matt Fleming <[email protected]>
L: [email protected]
T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
S: Maintained
F: Documentation/efi-stub.txt
F: arch/ia64/kernel/efi.c
F: arch/x86/boot/compressed/eboot.[ch]
F: arch/x86/include/asm/efi.h
F: arch/x86/platform/efi/*
F: drivers/firmware/efi/*
F: include/linux/efi*.h
Thanks,
Xiaolong
>
>[1] https://lkml.org/lkml/2018/6/22/39
>
>Regards,
>Sai
>_______________________________________________
>kbuild-all mailing list
>[email protected]
>https://lists.01.org/mailman/listinfo/kbuild-all
On 27 June 2018 at 08:02, Ye Xiaolong <[email protected]> wrote:
> Hi,
>
> On 06/26, Prakhya, Sai Praneeth wrote:
>>>
>>> Hi Sai,
>>>
>>> Thank you for the patch! Yet something to improve:
>>>
>>> [auto build test ERROR on linus/master]
>>> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
>>> the wrong git tree, please drop us a note to help improve the system]
>>
>>Since efi_memmap_free() is a recently introduced API [1], please note that the patch
>>is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>
> I noticed the official efi tree recored in MAINTAINERS is git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
> are they identical?
>
Which version of MAINTAINERS did you look at?
> EXTENSIBLE FIRMWARE INTERFACE (EFI)
> M: Matt Fleming <[email protected]>
> L: [email protected]
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
> S: Maintained
> F: Documentation/efi-stub.txt
> F: arch/ia64/kernel/efi.c
> F: arch/x86/boot/compressed/eboot.[ch]
> F: arch/x86/include/asm/efi.h
> F: arch/x86/platform/efi/*
> F: drivers/firmware/efi/*
> F: include/linux/efi*.h
>
>
> Thanks,
> Xiaolong
>
>>
>>[1] https://lkml.org/lkml/2018/6/22/39
>>
>>Regards,
>>Sai
>>_______________________________________________
>>kbuild-all mailing list
>>[email protected]
>>https://lists.01.org/mailman/listinfo/kbuild-all
On 06/27, Ard Biesheuvel wrote:
>On 27 June 2018 at 08:02, Ye Xiaolong <[email protected]> wrote:
>> Hi,
>>
>> On 06/26, Prakhya, Sai Praneeth wrote:
>>>>
>>>> Hi Sai,
>>>>
>>>> Thank you for the patch! Yet something to improve:
>>>>
>>>> [auto build test ERROR on linus/master]
>>>> [also build test ERROR on v4.18-rc2 next-20180625] [if your patch is applied to
>>>> the wrong git tree, please drop us a note to help improve the system]
>>>
>>>Since efi_memmap_free() is a recently introduced API [1], please note that the patch
>>>is based on efi tree @https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>>
>> I noticed the official efi tree recored in MAINTAINERS is git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
>> are they identical?
>>
>
>Which version of MAINTAINERS did you look at?
Oops, I just checked a old version. I'll add above efi.git tree to 0day's repo
pool.
Thanks,
Xiaolong
>
>> EXTENSIBLE FIRMWARE INTERFACE (EFI)
>> M: Matt Fleming <[email protected]>
>> L: [email protected]
>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
>> S: Maintained
>> F: Documentation/efi-stub.txt
>> F: arch/ia64/kernel/efi.c
>> F: arch/x86/boot/compressed/eboot.[ch]
>> F: arch/x86/include/asm/efi.h
>> F: arch/x86/platform/efi/*
>> F: drivers/firmware/efi/*
>> F: include/linux/efi*.h
>>
>>
>> Thanks,
>> Xiaolong
>>
>>>
>>>[1] https://lkml.org/lkml/2018/6/22/39
>>>
>>>Regards,
>>>Sai
>>>_______________________________________________
>>>kbuild-all mailing list
>>>[email protected]
>>>https://lists.01.org/mailman/listinfo/kbuild-all
On 27 June 2018 at 06:51, Prakhya, Sai Praneeth
<[email protected]> wrote:
>> > + /* Free the memory allocated to the existing memory map */
>> > + efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map,
>> > + efi.memmap.late);
>> > +
>> > data.phys_map = addr;
>> > data.size = efi.memmap.desc_size * nr_map;
>> > data.desc_version = efi.memmap.desc_version;
>> > --
>> > 2.7.4
>> >
>>
>> If only it were so simple :-)
>>
>> At this point, efi.memmap.phys_map could point to memory that was allocated
>> early, allocated late or simply passed to the OS at boot time by the stub (in
>> which case it was memblock_reserve()d but not memblock_alloc()d, and it
>> should not be freed)
>>
>
> Yes, completely agree that there could be three types of allocations for memmap.
> I thought,
> efi_memmap_free(efi.memmap.phys_map, efi.memmap.nr_map, efi.memmap.late);
>
> should work because the previous type of allocation should have been recorded in efi.memmap.late.
> But, now I see this will fail for memblock_reserved() memory because it will be mistaken to
> memblock_alloced() (I assumed both are almost similar :().
>
>> So only allocations made with efi_memmap_alloc() should be freed here.
>
> Makes sense and I think that also means efi_memmap_free() should be called from function
> that called efi_memmap_alloc() and not efi_memmap_install().
>
>> I'm not sure /how/ we should keep track of that: perhaps it is simply a matter of
>> replacing the boolean 'late' with an enum that describes where the memory
>> came from that phys_map points to.
>
> I did try changing boolean late to enum and it seems to be working fine. I will do more
> testing/clean up and will submit a patch for review.
>
> Also, could you please clarify if there is any specific reason why memory allocated
> using memblock_reserve() shouldn't be freed. I mean, not with memblock_free() but I
> think we could make it _available_ using free_bootmem() (or something similar, please
> correct me if this is not the right API).
On arm64, the memory map is provided to the core kernel by the stub,
and after kexec, a pointer to the same memory map will be passed to
the next kernel. So the kernel does not 'own' that allocation, and it
should not free it or overwrite it.
> If we allocate and install a new memory map (as
> in case with efi_fake_memmap()), I think we should free the memory used by memory map
> originally passed by EFI stub, because, at any point of time there should only be one active
> memory map. If we don't free the original memory map passed by EFI stub, we will be having
> two and hence will be leaking memory.
>
> Regards,
> Sai
> >Since efi_memmap_free() is a recently introduced API [1], please note
> >that the patch is based on efi tree
> >@https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
>
> I noticed the official efi tree recored in MAINTAINERS is
> git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git,
> are they identical?
>
> EXTENSIBLE FIRMWARE INTERFACE (EFI)
> M: Matt Fleming <[email protected]>
> L: [email protected]
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
> S: Maintained
> F: Documentation/efi-stub.txt
> F: arch/ia64/kernel/efi.c
> F: arch/x86/boot/compressed/eboot.[ch]
> F: arch/x86/include/asm/efi.h
> F: arch/x86/platform/efi/*
> F: drivers/firmware/efi/*
> F: include/linux/efi*.h
Hi Xiaolong,
Ard replaced Matt as EFI maintainer. Please see updated MAINTAINERS file for more details.
https://elixir.bootlin.com/linux/v4.18-rc2/source/MAINTAINERS
EXTENSIBLE FIRMWARE INTERFACE (EFI)
M: Ard Biesheuvel <[email protected]>
L: [email protected]
T: git git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
S: Maintained
F: Documentation/efi-stub.txt
F: arch/*/kernel/efi.c
F: arch/x86/boot/compressed/eboot.[ch]
F: arch/*/include/asm/efi.h
F: arch/x86/platform/efi/
F: drivers/firmware/efi/
F: include/linux/efi*.h
F: arch/arm/boot/compressed/efi-header.S
F: arch/arm64/kernel/efi-entry.S
Regards,
Sai
> > Also, could you please clarify if there is any specific reason why
> > memory allocated using memblock_reserve() shouldn't be freed. I mean,
> > not with memblock_free() but I think we could make it _available_
> > using free_bootmem() (or something similar, please correct me if this is not
> the right API).
>
> On arm64, the memory map is provided to the core kernel by the stub, and after
> kexec, a pointer to the same memory map will be passed to the next kernel. So
> the kernel does not 'own' that allocation, and it should not free it or overwrite
> it.
Thanks for the reply. It confirms that the issue is only on x86 systems.
I see that arm64 doesn't call efi_memmap_alloc() and hence there is no concept
of allocating memory for new memory map and installing it (so no memory leak).
Regards,
Sai