| Message ID | 20210429031602.2606654-4-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Further Strict RWX support | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (1b628dbc3ccb0b51963fcb3c60b57daac21dc016) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 74 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Le 29/04/2021 à 05:15, Jordan Niethe a écrit : > If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and > VMALLOC_END respectively. This reduces the need for special cases. For > example, powerpc's module_alloc() was previously predicated on > MODULES_VADDR being defined but now is unconditionally defined. > > This will be useful reducing conditional code in other places that need > to allocate from the module region (i.e., kprobes). > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v10: New to series > v11: - Consider more places MODULES_VADDR was being used > --- > arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ > arch/powerpc/kernel/module.c | 5 +---- > arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- > arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- > 4 files changed, 19 insertions(+), 11 deletions(-) > > diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h > index c6a676714f04..882fda779648 100644 > --- a/arch/powerpc/include/asm/pgtable.h > +++ b/arch/powerpc/include/asm/pgtable.h > @@ -39,6 +39,17 @@ struct mm_struct; > #define __S110 PAGE_SHARED_X > #define __S111 PAGE_SHARED_X > > +#ifndef MODULES_VADDR > +#define MODULES_VADDR VMALLOC_START > +#define MODULES_END VMALLOC_END > +#endif > + > +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) No no. TASK_SIZE > MODULES_VADDR is ALWAYS wrong, for any target, in any configuration. Why is it a problem to leave the test as a BUILD_BUG_ON() in module_alloc() ? > +#if TASK_SIZE > MODULES_VADDR > +#error TASK_SIZE > MODULES_VADDR > +#endif > +#endif > + > #ifndef __ASSEMBLY__ > > /* Keep these as a macros to avoid include dependency mess */ > diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c > index fab84024650c..c60c7457ff47 100644 > --- a/arch/powerpc/kernel/module.c > +++ b/arch/powerpc/kernel/module.c > @@ -15,6 +15,7 @@ > #include <linux/sort.h> > #include <asm/setup.h> > #include <asm/sections.h> > +#include <linux/mm.h> > > static LIST_HEAD(module_bug_list); > > @@ -88,7 +89,6 @@ int module_finalize(const Elf_Ehdr *hdr, > return 0; > } > > -#ifdef MODULES_VADDR > static __always_inline void * > __module_alloc(unsigned long size, unsigned long start, unsigned long end) > { > @@ -102,8 +102,6 @@ void *module_alloc(unsigned long size) > unsigned long limit = (unsigned long)_etext - SZ_32M; > void *ptr = NULL; > > - BUILD_BUG_ON(TASK_SIZE > MODULES_VADDR); > - > /* First try within 32M limit from _etext to avoid branch trampolines */ > if (MODULES_VADDR < PAGE_OFFSET && MODULES_END > limit) > ptr = __module_alloc(size, limit, MODULES_END); > @@ -113,4 +111,3 @@ void *module_alloc(unsigned long size) > > return ptr; > } > -#endif > diff --git a/arch/powerpc/mm/kasan/kasan_init_32.c b/arch/powerpc/mm/kasan/kasan_init_32.c > index cf8770b1a692..42c057366ac7 100644 > --- a/arch/powerpc/mm/kasan/kasan_init_32.c > +++ b/arch/powerpc/mm/kasan/kasan_init_32.c > @@ -116,11 +116,11 @@ static void __init kasan_unmap_early_shadow_vmalloc(void) > > kasan_update_early_region(k_start, k_end, __pte(0)); > > -#ifdef MODULES_VADDR > - k_start = (unsigned long)kasan_mem_to_shadow((void *)MODULES_VADDR); > - k_end = (unsigned long)kasan_mem_to_shadow((void *)MODULES_END); > - kasan_update_early_region(k_start, k_end, __pte(0)); > -#endif > + if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) { Shouldn't it be an || ? As soon as either MODULES_VADDR or MODULES_END differs from the vmalloc boundaries, it needs to be done I think. > + k_start = (unsigned long)kasan_mem_to_shadow((void *)MODULES_VADDR); > + k_end = (unsigned long)kasan_mem_to_shadow((void *)MODULES_END); > + kasan_update_early_region(k_start, k_end, __pte(0)); > + } > } > > void __init kasan_mmu_init(void) > diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c > index aca354fb670b..0431457f668f 100644 > --- a/arch/powerpc/mm/ptdump/ptdump.c > +++ b/arch/powerpc/mm/ptdump/ptdump.c > @@ -73,7 +73,7 @@ struct addr_marker { > > static struct addr_marker address_markers[] = { > { 0, "Start of kernel VM" }, > -#ifdef MODULES_VADDR > +#if defined(CONFIG_BOOK32_32) && defined(CONFIG_STRICT_KERNEL_RWX) Not valid anymore, see https://github.com/linuxppc/linux/commit/80edc68e0479 and https://github.com/linuxppc/linux/commit/9132a2e82adc The best would be to be able to do something like: #if MODULES_VADDR != VMALLOC_START If it doesn't work, then it has to be #if defined(CONFIG_BOOK32_32) || defined(CONFIG_PPC_8xx) > { 0, "modules start" }, > { 0, "modules end" }, > #endif > @@ -359,7 +359,7 @@ static void populate_markers(void) > #else > address_markers[i++].start_address = TASK_SIZE; > #endif > -#ifdef MODULES_VADDR > +#if defined(CONFIG_BOOK32_32) && defined(CONFIG_STRICT_KERNEL_RWX) Same. > address_markers[i++].start_address = MODULES_VADDR; > address_markers[i++].start_address = MODULES_END; > #endif >
On Thu, Apr 29, 2021 at 3:04 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 29/04/2021 à 05:15, Jordan Niethe a écrit : > > If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and > > VMALLOC_END respectively. This reduces the need for special cases. For > > example, powerpc's module_alloc() was previously predicated on > > MODULES_VADDR being defined but now is unconditionally defined. > > > > This will be useful reducing conditional code in other places that need > > to allocate from the module region (i.e., kprobes). > > > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > > --- > > v10: New to series > > v11: - Consider more places MODULES_VADDR was being used > > --- > > arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ > > arch/powerpc/kernel/module.c | 5 +---- > > arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- > > arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- > > 4 files changed, 19 insertions(+), 11 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h > > index c6a676714f04..882fda779648 100644 > > --- a/arch/powerpc/include/asm/pgtable.h > > +++ b/arch/powerpc/include/asm/pgtable.h > > @@ -39,6 +39,17 @@ struct mm_struct; > > #define __S110 PAGE_SHARED_X > > #define __S111 PAGE_SHARED_X > > > > +#ifndef MODULES_VADDR > > +#define MODULES_VADDR VMALLOC_START > > +#define MODULES_END VMALLOC_END > > +#endif > > + > > +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) > > No no. > > TASK_SIZE > MODULES_VADDR is ALWAYS wrong, for any target, in any configuration. > > Why is it a problem to leave the test as a BUILD_BUG_ON() in module_alloc() ? On ppc64s, MODULES_VADDR is __vmalloc_start (a variable) and TASK_SIZE depends on current. Also for nohash like 44x, MODULES_VADDR is defined based on high_memory. If I put it back in module_alloc() and wrap it with #ifdef CONFIG_PPC_BOOK3S_32 will that be fine? > > > +#if TASK_SIZE > MODULES_VADDR > > +#error TASK_SIZE > MODULES_VADDR > > +#endif > > +#endif > > + > > #ifndef __ASSEMBLY__ > > > > /* Keep these as a macros to avoid include dependency mess */ > > diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c > > index fab84024650c..c60c7457ff47 100644 > > --- a/arch/powerpc/kernel/module.c > > +++ b/arch/powerpc/kernel/module.c > > @@ -15,6 +15,7 @@ > > #include <linux/sort.h> > > #include <asm/setup.h> > > #include <asm/sections.h> > > +#include <linux/mm.h> > > > > static LIST_HEAD(module_bug_list); > > > > @@ -88,7 +89,6 @@ int module_finalize(const Elf_Ehdr *hdr, > > return 0; > > } > > > > -#ifdef MODULES_VADDR > > static __always_inline void * > > __module_alloc(unsigned long size, unsigned long start, unsigned long end) > > { > > @@ -102,8 +102,6 @@ void *module_alloc(unsigned long size) > > unsigned long limit = (unsigned long)_etext - SZ_32M; > > void *ptr = NULL; > > > > - BUILD_BUG_ON(TASK_SIZE > MODULES_VADDR); > > - > > /* First try within 32M limit from _etext to avoid branch trampolines */ > > if (MODULES_VADDR < PAGE_OFFSET && MODULES_END > limit) > > ptr = __module_alloc(size, limit, MODULES_END); > > @@ -113,4 +111,3 @@ void *module_alloc(unsigned long size) > > > > return ptr; > > } > > -#endif > > diff --git a/arch/powerpc/mm/kasan/kasan_init_32.c b/arch/powerpc/mm/kasan/kasan_init_32.c > > index cf8770b1a692..42c057366ac7 100644 > > --- a/arch/powerpc/mm/kasan/kasan_init_32.c > > +++ b/arch/powerpc/mm/kasan/kasan_init_32.c > > @@ -116,11 +116,11 @@ static void __init kasan_unmap_early_shadow_vmalloc(void) > > > > kasan_update_early_region(k_start, k_end, __pte(0)); > > > > -#ifdef MODULES_VADDR > > - k_start = (unsigned long)kasan_mem_to_shadow((void *)MODULES_VADDR); > > - k_end = (unsigned long)kasan_mem_to_shadow((void *)MODULES_END); > > - kasan_update_early_region(k_start, k_end, __pte(0)); > > -#endif > > + if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) { > > Shouldn't it be an || ? Yeah > > As soon as either MODULES_VADDR or MODULES_END differs from the vmalloc boundaries, it needs to be > done I think. > > > + k_start = (unsigned long)kasan_mem_to_shadow((void *)MODULES_VADDR); > > + k_end = (unsigned long)kasan_mem_to_shadow((void *)MODULES_END); > > + kasan_update_early_region(k_start, k_end, __pte(0)); > > + } > > } > > > > void __init kasan_mmu_init(void) > > diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c > > index aca354fb670b..0431457f668f 100644 > > --- a/arch/powerpc/mm/ptdump/ptdump.c > > +++ b/arch/powerpc/mm/ptdump/ptdump.c > > @@ -73,7 +73,7 @@ struct addr_marker { > > > > static struct addr_marker address_markers[] = { > > { 0, "Start of kernel VM" }, > > -#ifdef MODULES_VADDR > > +#if defined(CONFIG_BOOK32_32) && defined(CONFIG_STRICT_KERNEL_RWX) > > Not valid anymore, see https://github.com/linuxppc/linux/commit/80edc68e0479 and > https://github.com/linuxppc/linux/commit/9132a2e82adc > > The best would be to be able to do something like: > > #if MODULES_VADDR != VMALLOC_START I tried to do it like that originally but with stuff like #define VMALLOC_START ((((long)high_memory + VMALLOC_OFFSET) & ~(VMALLOC_OFFSET-1))) it doesn't work. > > If it doesn't work, then it has to be > > #if defined(CONFIG_BOOK32_32) || defined(CONFIG_PPC_8xx) Ok, thanks. > > > { 0, "modules start" }, > > { 0, "modules end" }, > > #endif > > @@ -359,7 +359,7 @@ static void populate_markers(void) > > #else > > address_markers[i++].start_address = TASK_SIZE; > > #endif > > -#ifdef MODULES_VADDR > > +#if defined(CONFIG_BOOK32_32) && defined(CONFIG_STRICT_KERNEL_RWX) > > Same. > > > address_markers[i++].start_address = MODULES_VADDR; > > address_markers[i++].start_address = MODULES_END; > > #endif > >
Le 03/05/2021 à 07:39, Jordan Niethe a écrit : > On Thu, Apr 29, 2021 at 3:04 PM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> >> >> Le 29/04/2021 à 05:15, Jordan Niethe a écrit : >>> If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and >>> VMALLOC_END respectively. This reduces the need for special cases. For >>> example, powerpc's module_alloc() was previously predicated on >>> MODULES_VADDR being defined but now is unconditionally defined. >>> >>> This will be useful reducing conditional code in other places that need >>> to allocate from the module region (i.e., kprobes). >>> >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> >>> --- >>> v10: New to series >>> v11: - Consider more places MODULES_VADDR was being used >>> --- >>> arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ >>> arch/powerpc/kernel/module.c | 5 +---- >>> arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- >>> arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- >>> 4 files changed, 19 insertions(+), 11 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h >>> index c6a676714f04..882fda779648 100644 >>> --- a/arch/powerpc/include/asm/pgtable.h >>> +++ b/arch/powerpc/include/asm/pgtable.h >>> @@ -39,6 +39,17 @@ struct mm_struct; >>> #define __S110 PAGE_SHARED_X >>> #define __S111 PAGE_SHARED_X >>> >>> +#ifndef MODULES_VADDR >>> +#define MODULES_VADDR VMALLOC_START >>> +#define MODULES_END VMALLOC_END >>> +#endif >>> + >>> +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) >> >> No no. >> >> TASK_SIZE > MODULES_VADDR is ALWAYS wrong, for any target, in any configuration. >> >> Why is it a problem to leave the test as a BUILD_BUG_ON() in module_alloc() ? > On ppc64s, MODULES_VADDR is __vmalloc_start (a variable) and > TASK_SIZE depends on current. > Also for nohash like 44x, MODULES_VADDR is defined based on high_memory. > If I put it back in module_alloc() and wrap it with #ifdef > CONFIG_PPC_BOOK3S_32 will that be fine? Thinking about it once more, I think the best approach is the one taken by Nick in https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210502110050.324953-1-npiggin@gmail.com/ Use MODULES_VADDR/MODULES_END when it exists, use VMALLOC_START/VMALLOC_END otherwise. I know I suggested to always define MODULES_VADDR, but maybe that's not the best solution at the end. For kprobes, is there a way to re-use functions from modules.c in alloc_insn_page() ? Christophe
On Mon, May 3, 2021 at 3:57 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 03/05/2021 à 07:39, Jordan Niethe a écrit : > > On Thu, Apr 29, 2021 at 3:04 PM Christophe Leroy > > <christophe.leroy@csgroup.eu> wrote: > >> > >> > >> > >> Le 29/04/2021 à 05:15, Jordan Niethe a écrit : > >>> If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and > >>> VMALLOC_END respectively. This reduces the need for special cases. For > >>> example, powerpc's module_alloc() was previously predicated on > >>> MODULES_VADDR being defined but now is unconditionally defined. > >>> > >>> This will be useful reducing conditional code in other places that need > >>> to allocate from the module region (i.e., kprobes). > >>> > >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > >>> --- > >>> v10: New to series > >>> v11: - Consider more places MODULES_VADDR was being used > >>> --- > >>> arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ > >>> arch/powerpc/kernel/module.c | 5 +---- > >>> arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- > >>> arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- > >>> 4 files changed, 19 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h > >>> index c6a676714f04..882fda779648 100644 > >>> --- a/arch/powerpc/include/asm/pgtable.h > >>> +++ b/arch/powerpc/include/asm/pgtable.h > >>> @@ -39,6 +39,17 @@ struct mm_struct; > >>> #define __S110 PAGE_SHARED_X > >>> #define __S111 PAGE_SHARED_X > >>> > >>> +#ifndef MODULES_VADDR > >>> +#define MODULES_VADDR VMALLOC_START > >>> +#define MODULES_END VMALLOC_END > >>> +#endif > >>> + > >>> +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) > >> > >> No no. > >> > >> TASK_SIZE > MODULES_VADDR is ALWAYS wrong, for any target, in any configuration. > >> > >> Why is it a problem to leave the test as a BUILD_BUG_ON() in module_alloc() ? > > On ppc64s, MODULES_VADDR is __vmalloc_start (a variable) and > > TASK_SIZE depends on current. > > Also for nohash like 44x, MODULES_VADDR is defined based on high_memory. > > If I put it back in module_alloc() and wrap it with #ifdef > > CONFIG_PPC_BOOK3S_32 will that be fine? > > Thinking about it once more, I think the best approach is the one taken by Nick in > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210502110050.324953-1-npiggin@gmail.com/ > > Use MODULES_VADDR/MODULES_END when it exists, use VMALLOC_START/VMALLOC_END otherwise. > > I know I suggested to always define MODULES_VADDR, but maybe that's not the best solution at the end. Sure, let's do it like that. > > For kprobes, is there a way to re-use functions from modules.c in alloc_insn_page() ? Probably we can use module_alloc() then the set_memory_ functions to get the permissions right. Something like we had in v9: https://lore.kernel.org/linuxppc-dev/20210316031741.1004850-3-jniethe5@gmail.com/ > > Christophe
Le 03/05/2021 à 08:16, Jordan Niethe a écrit : > On Mon, May 3, 2021 at 3:57 PM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> >> >> Le 03/05/2021 à 07:39, Jordan Niethe a écrit : >>> On Thu, Apr 29, 2021 at 3:04 PM Christophe Leroy >>> <christophe.leroy@csgroup.eu> wrote: >>>> >>>> >>>> >>>> Le 29/04/2021 à 05:15, Jordan Niethe a écrit : >>>>> If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and >>>>> VMALLOC_END respectively. This reduces the need for special cases. For >>>>> example, powerpc's module_alloc() was previously predicated on >>>>> MODULES_VADDR being defined but now is unconditionally defined. >>>>> >>>>> This will be useful reducing conditional code in other places that need >>>>> to allocate from the module region (i.e., kprobes). >>>>> >>>>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> >>>>> --- >>>>> v10: New to series >>>>> v11: - Consider more places MODULES_VADDR was being used >>>>> --- >>>>> arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ >>>>> arch/powerpc/kernel/module.c | 5 +---- >>>>> arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- >>>>> arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- >>>>> 4 files changed, 19 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h >>>>> index c6a676714f04..882fda779648 100644 >>>>> --- a/arch/powerpc/include/asm/pgtable.h >>>>> +++ b/arch/powerpc/include/asm/pgtable.h >>>>> @@ -39,6 +39,17 @@ struct mm_struct; >>>>> #define __S110 PAGE_SHARED_X >>>>> #define __S111 PAGE_SHARED_X >>>>> >>>>> +#ifndef MODULES_VADDR >>>>> +#define MODULES_VADDR VMALLOC_START >>>>> +#define MODULES_END VMALLOC_END >>>>> +#endif >>>>> + >>>>> +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) >>>> >>>> No no. >>>> >>>> TASK_SIZE > MODULES_VADDR is ALWAYS wrong, for any target, in any configuration. >>>> >>>> Why is it a problem to leave the test as a BUILD_BUG_ON() in module_alloc() ? >>> On ppc64s, MODULES_VADDR is __vmalloc_start (a variable) and >>> TASK_SIZE depends on current. >>> Also for nohash like 44x, MODULES_VADDR is defined based on high_memory. >>> If I put it back in module_alloc() and wrap it with #ifdef >>> CONFIG_PPC_BOOK3S_32 will that be fine? >> >> Thinking about it once more, I think the best approach is the one taken by Nick in >> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210502110050.324953-1-npiggin@gmail.com/ >> >> Use MODULES_VADDR/MODULES_END when it exists, use VMALLOC_START/VMALLOC_END otherwise. >> >> I know I suggested to always define MODULES_VADDR, but maybe that's not the best solution at the end. > Sure, let's do it like that. >> >> For kprobes, is there a way to re-use functions from modules.c in alloc_insn_page() ? > Probably we can use module_alloc() then the set_memory_ functions to > get the permissions right. > Something like we had in v9: > https://lore.kernel.org/linuxppc-dev/20210316031741.1004850-3-jniethe5@gmail.com/ Yes, more or less, but using module_alloc() instead of vmalloc(). And module_alloc() implies EXEC, so only the set_memory_ro() will be required. I see no point in doing any set_memory_xxx() in free_insn_page(), because as soon as you do a vfree() the page is not mapped anymore so any access will lead to a fault. Christophe
On Mon, May 3, 2021 at 4:22 PM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 03/05/2021 à 08:16, Jordan Niethe a écrit : > > On Mon, May 3, 2021 at 3:57 PM Christophe Leroy > > <christophe.leroy@csgroup.eu> wrote: > >> > >> > >> > >> Le 03/05/2021 à 07:39, Jordan Niethe a écrit : > >>> On Thu, Apr 29, 2021 at 3:04 PM Christophe Leroy > >>> <christophe.leroy@csgroup.eu> wrote: > >>>> > >>>> > >>>> > >>>> Le 29/04/2021 à 05:15, Jordan Niethe a écrit : > >>>>> If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and > >>>>> VMALLOC_END respectively. This reduces the need for special cases. For > >>>>> example, powerpc's module_alloc() was previously predicated on > >>>>> MODULES_VADDR being defined but now is unconditionally defined. > >>>>> > >>>>> This will be useful reducing conditional code in other places that need > >>>>> to allocate from the module region (i.e., kprobes). > >>>>> > >>>>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > >>>>> --- > >>>>> v10: New to series > >>>>> v11: - Consider more places MODULES_VADDR was being used > >>>>> --- > >>>>> arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ > >>>>> arch/powerpc/kernel/module.c | 5 +---- > >>>>> arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- > >>>>> arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- > >>>>> 4 files changed, 19 insertions(+), 11 deletions(-) > >>>>> > >>>>> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h > >>>>> index c6a676714f04..882fda779648 100644 > >>>>> --- a/arch/powerpc/include/asm/pgtable.h > >>>>> +++ b/arch/powerpc/include/asm/pgtable.h > >>>>> @@ -39,6 +39,17 @@ struct mm_struct; > >>>>> #define __S110 PAGE_SHARED_X > >>>>> #define __S111 PAGE_SHARED_X > >>>>> > >>>>> +#ifndef MODULES_VADDR > >>>>> +#define MODULES_VADDR VMALLOC_START > >>>>> +#define MODULES_END VMALLOC_END > >>>>> +#endif > >>>>> + > >>>>> +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) > >>>> > >>>> No no. > >>>> > >>>> TASK_SIZE > MODULES_VADDR is ALWAYS wrong, for any target, in any configuration. > >>>> > >>>> Why is it a problem to leave the test as a BUILD_BUG_ON() in module_alloc() ? > >>> On ppc64s, MODULES_VADDR is __vmalloc_start (a variable) and > >>> TASK_SIZE depends on current. > >>> Also for nohash like 44x, MODULES_VADDR is defined based on high_memory. > >>> If I put it back in module_alloc() and wrap it with #ifdef > >>> CONFIG_PPC_BOOK3S_32 will that be fine? > >> > >> Thinking about it once more, I think the best approach is the one taken by Nick in > >> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210502110050.324953-1-npiggin@gmail.com/ > >> > >> Use MODULES_VADDR/MODULES_END when it exists, use VMALLOC_START/VMALLOC_END otherwise. > >> > >> I know I suggested to always define MODULES_VADDR, but maybe that's not the best solution at the end. > > Sure, let's do it like that. > >> > >> For kprobes, is there a way to re-use functions from modules.c in alloc_insn_page() ? > > Probably we can use module_alloc() then the set_memory_ functions to > > get the permissions right. > > Something like we had in v9: > > https://lore.kernel.org/linuxppc-dev/20210316031741.1004850-3-jniethe5@gmail.com/ > > Yes, more or less, but using module_alloc() instead of vmalloc(). > And module_alloc() implies EXEC, so only the set_memory_ro() will be required. Yep. > > I see no point in doing any set_memory_xxx() in free_insn_page(), because as soon as you do a > vfree() the page is not mapped anymore so any access will lead to a fault. Yeah, I'd not realised we had VM_FLUSH_RESET_PERMS when I added that. I agree it's pointless. > > Christophe
Le 03/05/2021 à 08:26, Jordan Niethe a écrit : > On Mon, May 3, 2021 at 4:22 PM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> >> >> Le 03/05/2021 à 08:16, Jordan Niethe a écrit : >>> On Mon, May 3, 2021 at 3:57 PM Christophe Leroy >>> <christophe.leroy@csgroup.eu> wrote: >>>> >>>> >>>> >>>> Le 03/05/2021 à 07:39, Jordan Niethe a écrit : >>>>> On Thu, Apr 29, 2021 at 3:04 PM Christophe Leroy >>>>> <christophe.leroy@csgroup.eu> wrote: >>>>>> >>>>>> >>>>>> >>>>>> Le 29/04/2021 à 05:15, Jordan Niethe a écrit : >>>>>>> If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and >>>>>>> VMALLOC_END respectively. This reduces the need for special cases. For >>>>>>> example, powerpc's module_alloc() was previously predicated on >>>>>>> MODULES_VADDR being defined but now is unconditionally defined. >>>>>>> >>>>>>> This will be useful reducing conditional code in other places that need >>>>>>> to allocate from the module region (i.e., kprobes). >>>>>>> >>>>>>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com> >>>>>>> --- >>>>>>> v10: New to series >>>>>>> v11: - Consider more places MODULES_VADDR was being used >>>>>>> --- >>>>>>> arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ >>>>>>> arch/powerpc/kernel/module.c | 5 +---- >>>>>>> arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- >>>>>>> arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- >>>>>>> 4 files changed, 19 insertions(+), 11 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h >>>>>>> index c6a676714f04..882fda779648 100644 >>>>>>> --- a/arch/powerpc/include/asm/pgtable.h >>>>>>> +++ b/arch/powerpc/include/asm/pgtable.h >>>>>>> @@ -39,6 +39,17 @@ struct mm_struct; >>>>>>> #define __S110 PAGE_SHARED_X >>>>>>> #define __S111 PAGE_SHARED_X >>>>>>> >>>>>>> +#ifndef MODULES_VADDR >>>>>>> +#define MODULES_VADDR VMALLOC_START >>>>>>> +#define MODULES_END VMALLOC_END >>>>>>> +#endif >>>>>>> + >>>>>>> +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) >>>>>> >>>>>> No no. >>>>>> >>>>>> TASK_SIZE > MODULES_VADDR is ALWAYS wrong, for any target, in any configuration. >>>>>> >>>>>> Why is it a problem to leave the test as a BUILD_BUG_ON() in module_alloc() ? >>>>> On ppc64s, MODULES_VADDR is __vmalloc_start (a variable) and >>>>> TASK_SIZE depends on current. >>>>> Also for nohash like 44x, MODULES_VADDR is defined based on high_memory. >>>>> If I put it back in module_alloc() and wrap it with #ifdef >>>>> CONFIG_PPC_BOOK3S_32 will that be fine? >>>> >>>> Thinking about it once more, I think the best approach is the one taken by Nick in >>>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210502110050.324953-1-npiggin@gmail.com/ >>>> >>>> Use MODULES_VADDR/MODULES_END when it exists, use VMALLOC_START/VMALLOC_END otherwise. >>>> >>>> I know I suggested to always define MODULES_VADDR, but maybe that's not the best solution at the end. >>> Sure, let's do it like that. >>>> >>>> For kprobes, is there a way to re-use functions from modules.c in alloc_insn_page() ? >>> Probably we can use module_alloc() then the set_memory_ functions to >>> get the permissions right. >>> Something like we had in v9: >>> https://lore.kernel.org/linuxppc-dev/20210316031741.1004850-3-jniethe5@gmail.com/ >> >> Yes, more or less, but using module_alloc() instead of vmalloc(). >> And module_alloc() implies EXEC, so only the set_memory_ro() will be required. > Yep. >> >> I see no point in doing any set_memory_xxx() in free_insn_page(), because as soon as you do a >> vfree() the page is not mapped anymore so any access will lead to a fault. > Yeah, I'd not realised we had VM_FLUSH_RESET_PERMS when I added that. > I agree it's pointless. At the end if should be quite similar to what S390 architecture does.
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h index c6a676714f04..882fda779648 100644 --- a/arch/powerpc/include/asm/pgtable.h +++ b/arch/powerpc/include/asm/pgtable.h @@ -39,6 +39,17 @@ struct mm_struct; #define __S110 PAGE_SHARED_X #define __S111 PAGE_SHARED_X +#ifndef MODULES_VADDR +#define MODULES_VADDR VMALLOC_START +#define MODULES_END VMALLOC_END +#endif + +#if defined(CONFIG_PPC_BOOK3S_32) && defined(CONFIG_STRICT_KERNEL_RWX) +#if TASK_SIZE > MODULES_VADDR +#error TASK_SIZE > MODULES_VADDR +#endif +#endif + #ifndef __ASSEMBLY__ /* Keep these as a macros to avoid include dependency mess */ diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c index fab84024650c..c60c7457ff47 100644 --- a/arch/powerpc/kernel/module.c +++ b/arch/powerpc/kernel/module.c @@ -15,6 +15,7 @@ #include <linux/sort.h> #include <asm/setup.h> #include <asm/sections.h> +#include <linux/mm.h> static LIST_HEAD(module_bug_list); @@ -88,7 +89,6 @@ int module_finalize(const Elf_Ehdr *hdr, return 0; } -#ifdef MODULES_VADDR static __always_inline void * __module_alloc(unsigned long size, unsigned long start, unsigned long end) { @@ -102,8 +102,6 @@ void *module_alloc(unsigned long size) unsigned long limit = (unsigned long)_etext - SZ_32M; void *ptr = NULL; - BUILD_BUG_ON(TASK_SIZE > MODULES_VADDR); - /* First try within 32M limit from _etext to avoid branch trampolines */ if (MODULES_VADDR < PAGE_OFFSET && MODULES_END > limit) ptr = __module_alloc(size, limit, MODULES_END); @@ -113,4 +111,3 @@ void *module_alloc(unsigned long size) return ptr; } -#endif diff --git a/arch/powerpc/mm/kasan/kasan_init_32.c b/arch/powerpc/mm/kasan/kasan_init_32.c index cf8770b1a692..42c057366ac7 100644 --- a/arch/powerpc/mm/kasan/kasan_init_32.c +++ b/arch/powerpc/mm/kasan/kasan_init_32.c @@ -116,11 +116,11 @@ static void __init kasan_unmap_early_shadow_vmalloc(void) kasan_update_early_region(k_start, k_end, __pte(0)); -#ifdef MODULES_VADDR - k_start = (unsigned long)kasan_mem_to_shadow((void *)MODULES_VADDR); - k_end = (unsigned long)kasan_mem_to_shadow((void *)MODULES_END); - kasan_update_early_region(k_start, k_end, __pte(0)); -#endif + if (MODULES_VADDR != VMALLOC_START && MODULES_END != VMALLOC_END) { + k_start = (unsigned long)kasan_mem_to_shadow((void *)MODULES_VADDR); + k_end = (unsigned long)kasan_mem_to_shadow((void *)MODULES_END); + kasan_update_early_region(k_start, k_end, __pte(0)); + } } void __init kasan_mmu_init(void) diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c index aca354fb670b..0431457f668f 100644 --- a/arch/powerpc/mm/ptdump/ptdump.c +++ b/arch/powerpc/mm/ptdump/ptdump.c @@ -73,7 +73,7 @@ struct addr_marker { static struct addr_marker address_markers[] = { { 0, "Start of kernel VM" }, -#ifdef MODULES_VADDR +#if defined(CONFIG_BOOK32_32) && defined(CONFIG_STRICT_KERNEL_RWX) { 0, "modules start" }, { 0, "modules end" }, #endif @@ -359,7 +359,7 @@ static void populate_markers(void) #else address_markers[i++].start_address = TASK_SIZE; #endif -#ifdef MODULES_VADDR +#if defined(CONFIG_BOOK32_32) && defined(CONFIG_STRICT_KERNEL_RWX) address_markers[i++].start_address = MODULES_VADDR; address_markers[i++].start_address = MODULES_END; #endif
If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and VMALLOC_END respectively. This reduces the need for special cases. For example, powerpc's module_alloc() was previously predicated on MODULES_VADDR being defined but now is unconditionally defined. This will be useful reducing conditional code in other places that need to allocate from the module region (i.e., kprobes). Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v10: New to series v11: - Consider more places MODULES_VADDR was being used --- arch/powerpc/include/asm/pgtable.h | 11 +++++++++++ arch/powerpc/kernel/module.c | 5 +---- arch/powerpc/mm/kasan/kasan_init_32.c | 10 +++++----- arch/powerpc/mm/ptdump/ptdump.c | 4 ++-- 4 files changed, 19 insertions(+), 11 deletions(-)