| Message ID | 0b55650058a5bf64f7d74781871a1ada2298c8b4.1632491308.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] mm: Make generic arch_is_kernel_initmem_freed() do what it says | expand |
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Commit 7a5da02de8d6 ("locking/lockdep: check for freed initmem in > static_obj()") added arch_is_kernel_initmem_freed() which is supposed > to report whether an object is part of already freed init memory. > > For the time being, the generic version of arch_is_kernel_initmem_freed() > always reports 'false', allthough free_initmem() is generically called > on all architectures. > > Therefore, change the generic version of arch_is_kernel_initmem_freed() > to check whether free_initmem() has been called. If so, then check > if a given address falls into init memory. > > In order to use function init_section_contains(), the fonction is > moved at the end of asm-generic/section.h > > Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > include/asm-generic/sections.h | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h > index d16302d3eb59..d1e5bb2c6b72 100644 > --- a/include/asm-generic/sections.h > +++ b/include/asm-generic/sections.h > @@ -172,4 +158,21 @@ static inline bool is_kernel_rodata(unsigned long addr) > addr < (unsigned long)__end_rodata; > } > > +/* > + * Check if an address is part of freed initmem. This is needed on architectures > + * with virt == phys kernel mapping, for code that wants to check if an address > + * is part of a static object within [_stext, _end]. After initmem is freed, > + * memory can be allocated from it, and such allocations would then have > + * addresses within the range [_stext, _end]. > + */ > +#ifndef arch_is_kernel_initmem_freed > +static inline int arch_is_kernel_initmem_freed(unsigned long addr) > +{ > + if (system_state < SYSTEM_RUNNING) > + return 0; > + > + return init_section_contains((void *)addr, 1); > +} > +#endif This will return an incorrect result for a short period during boot won't it? See init/main.c: static int __ref kernel_init(void *unused) { ... free_initmem(); <- memory is freed here mark_readonly(); /* * Kernel mappings are now finalized - update the userspace page-table * to finalize PTI. */ pti_finalize(); system_state = SYSTEM_RUNNING; After free_initmem() we have address ranges that are now freed initmem, but arch_is_kernel_initmem_freed() continues to return 0 (false) for all addresses, until we update system_state. Possibly that doesn't matter for any of the current callers, but it seems pretty dicey to me. cheers
Le 27/09/2021 à 15:11, Michael Ellerman a écrit : > Christophe Leroy <christophe.leroy@csgroup.eu> writes: >> Commit 7a5da02de8d6 ("locking/lockdep: check for freed initmem in >> static_obj()") added arch_is_kernel_initmem_freed() which is supposed >> to report whether an object is part of already freed init memory. >> >> For the time being, the generic version of arch_is_kernel_initmem_freed() >> always reports 'false', allthough free_initmem() is generically called >> on all architectures. >> >> Therefore, change the generic version of arch_is_kernel_initmem_freed() >> to check whether free_initmem() has been called. If so, then check >> if a given address falls into init memory. >> >> In order to use function init_section_contains(), the fonction is >> moved at the end of asm-generic/section.h >> >> Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> include/asm-generic/sections.h | 31 +++++++++++++++++-------------- >> 1 file changed, 17 insertions(+), 14 deletions(-) >> >> diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h >> index d16302d3eb59..d1e5bb2c6b72 100644 >> --- a/include/asm-generic/sections.h >> +++ b/include/asm-generic/sections.h >> @@ -172,4 +158,21 @@ static inline bool is_kernel_rodata(unsigned long addr) >> addr < (unsigned long)__end_rodata; >> } >> >> +/* >> + * Check if an address is part of freed initmem. This is needed on architectures >> + * with virt == phys kernel mapping, for code that wants to check if an address >> + * is part of a static object within [_stext, _end]. After initmem is freed, >> + * memory can be allocated from it, and such allocations would then have >> + * addresses within the range [_stext, _end]. >> + */ >> +#ifndef arch_is_kernel_initmem_freed >> +static inline int arch_is_kernel_initmem_freed(unsigned long addr) >> +{ >> + if (system_state < SYSTEM_RUNNING) >> + return 0; >> + >> + return init_section_contains((void *)addr, 1); >> +} >> +#endif > > This will return an incorrect result for a short period during boot > won't it? > > See init/main.c: > > static int __ref kernel_init(void *unused) > { > ... > free_initmem(); <- memory is freed here > mark_readonly(); > > /* > * Kernel mappings are now finalized - update the userspace page-table > * to finalize PTI. > */ > pti_finalize(); > > system_state = SYSTEM_RUNNING; > > > After free_initmem() we have address ranges that are now freed initmem, > but arch_is_kernel_initmem_freed() continues to return 0 (false) for all > addresses, until we update system_state. > > Possibly that doesn't matter for any of the current callers, but it > seems pretty dicey to me. > Yes I saw it but as function core_kernel_text() uses that criteria for deciding whether a given init text address is valid or not, I thought it was just ok. Should we add an intermediate state called for exemple SYSTEM_FREEING_INIT just before SYSTEM_RUNNING ? Christophe
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index d16302d3eb59..d1e5bb2c6b72 100644 --- a/include/asm-generic/sections.h +++ b/include/asm-generic/sections.h @@ -80,20 +80,6 @@ static inline int arch_is_kernel_data(unsigned long addr) } #endif -/* - * Check if an address is part of freed initmem. This is needed on architectures - * with virt == phys kernel mapping, for code that wants to check if an address - * is part of a static object within [_stext, _end]. After initmem is freed, - * memory can be allocated from it, and such allocations would then have - * addresses within the range [_stext, _end]. - */ -#ifndef arch_is_kernel_initmem_freed -static inline int arch_is_kernel_initmem_freed(unsigned long addr) -{ - return 0; -} -#endif - /** * memory_contains - checks if an object is contained within a memory region * @begin: virtual address of the beginning of the memory region @@ -172,4 +158,21 @@ static inline bool is_kernel_rodata(unsigned long addr) addr < (unsigned long)__end_rodata; } +/* + * Check if an address is part of freed initmem. This is needed on architectures + * with virt == phys kernel mapping, for code that wants to check if an address + * is part of a static object within [_stext, _end]. After initmem is freed, + * memory can be allocated from it, and such allocations would then have + * addresses within the range [_stext, _end]. + */ +#ifndef arch_is_kernel_initmem_freed +static inline int arch_is_kernel_initmem_freed(unsigned long addr) +{ + if (system_state < SYSTEM_RUNNING) + return 0; + + return init_section_contains((void *)addr, 1); +} +#endif + #endif /* _ASM_GENERIC_SECTIONS_H_ */
Commit 7a5da02de8d6 ("locking/lockdep: check for freed initmem in static_obj()") added arch_is_kernel_initmem_freed() which is supposed to report whether an object is part of already freed init memory. For the time being, the generic version of arch_is_kernel_initmem_freed() always reports 'false', allthough free_initmem() is generically called on all architectures. Therefore, change the generic version of arch_is_kernel_initmem_freed() to check whether free_initmem() has been called. If so, then check if a given address falls into init memory. In order to use function init_section_contains(), the fonction is moved at the end of asm-generic/section.h Cc: Gerald Schaefer <gerald.schaefer@linux.ibm.com> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- include/asm-generic/sections.h | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)