RE: [PATCH] efi: Free existing memory map before installing new memory map
> > 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
Re: [PATCH] efi: Free existing memory map before installing new memory map
On 27 June 2018 at 06:51, Prakhya, Sai Praneeth 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 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
On 06/27, Ard Biesheuvel wrote: >On 27 June 2018 at 08:02, Ye Xiaolong 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 >> L: linux-efi@vger.kernel.org >> 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 >>>kbuild-...@lists.01.org >>>https://lists.01.org/mailman/listinfo/kbuild-all -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
> >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 > L: linux-efi@vger.kernel.org > 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 L: linux-efi@vger.kernel.org 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 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
On 27 June 2018 at 08:02, Ye Xiaolong 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 > L: linux-efi@vger.kernel.org > 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 >>kbuild-...@lists.01.org >>https://lists.01.org/mailman/listinfo/kbuild-all -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [kbuild-all] [PATCH] efi: Free existing memory map before installing new memory map
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 L: linux-efi@vger.kernel.org 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 >kbuild-...@lists.01.org >https://lists.01.org/mailman/listinfo/kbuild-all -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] efi: Free existing memory map before installing new memory map
> > + /* 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
Re: [PATCH] efi: Free existing memory map before installing new memory map
On 26 June 2018 at 04:41, Sai Praneeth Prakhya wrote: > From: Sai Praneeth > > 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 > Reported-by: Ard Biesheuvel > Cc: Lee Chun-Yi > Cc: Borislav Petkov > Cc: Dave Young > Cc: Laszlo Ersek > Cc: Bhupesh Sharma > Cc: Ricardo Neri > Cc: Ravi Shankar > Cc: Matt Fleming > Cc: Ard Biesheuvel > --- > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] efi: Free existing memory map before installing new memory map
> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] efi: Free existing memory map before installing new memory map
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(, efi.memmap.late);
207 }
208
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH] efi: Free existing memory map before installing new memory map
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(, efi.memmap.late);
207 }
208
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
[PATCH] efi: Free existing memory map before installing new memory map
From: Sai Praneeth 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 Reported-by: Ard Biesheuvel Cc: Lee Chun-Yi Cc: Borislav Petkov Cc: Dave Young Cc: Laszlo Ersek Cc: Bhupesh Sharma Cc: Ricardo Neri Cc: Ravi Shankar Cc: Matt Fleming Cc: Ard Biesheuvel --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
