| Message ID | 20231023075714.21672-1-quic_aiquny@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: module: PLT allowed even !RANDOM_BASE | expand |
On Mon, Oct 23, 2023, at 09:57, Maria Yu wrote: > Module PLT feature can be enabled even when RANDOM_BASE is disabled. > Break BLT entry counts of relocation types will make module plt entry > allocation fail and finally exec format error for even correct and plt > allocation available modules. > > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> Adding Ard Biesheuvel to Cc, as he added the check in commit a257e02579e42 ("arm64/kernel: don't ban ADRP to work around Cortex-A53 erratum #843419") > arch/arm64/kernel/module-plts.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/arch/arm64/kernel/module-plts.c > b/arch/arm64/kernel/module-plts.c > index bd69a4e7cd60..21a67d52d7a0 100644 > --- a/arch/arm64/kernel/module-plts.c > +++ b/arch/arm64/kernel/module-plts.c > @@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms, > Elf64_Rela *rela, int num, > switch (ELF64_R_TYPE(rela[i].r_info)) { > case R_AARCH64_JUMP26: > case R_AARCH64_CALL26: > - if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE)) > - break; > - > /* > * We only have to consider branch targets that resolve > * to symbols that are defined in a different section. I see there are two such checks (in partition_branch_plt_relas() and in count_plts()), can you explain in more detail how you concluded that one of them is correct but the other one is not? Arnd
On Mon, Oct 23, 2023 at 10:08:33AM +0200, Arnd Bergmann wrote: > On Mon, Oct 23, 2023, at 09:57, Maria Yu wrote: > > Module PLT feature can be enabled even when RANDOM_BASE is disabled. > > Break BLT entry counts of relocation types will make module plt entry > > allocation fail and finally exec format error for even correct and plt > > allocation available modules. Has an actual problem been seen in practice, or was this found by looking at the code? > > > > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > > Adding Ard Biesheuvel to Cc, as he added the check in commit > a257e02579e42 ("arm64/kernel: don't ban ADRP to work around > Cortex-A53 erratum #843419") I think that the actual mistake is in commit: 3e35d303ab7d22c4 ("arm64: module: rework module VA range selection") Prior to that commit, when CONFIG_RANDOMIZE_BASE=n all modules and code had to be within 128M of each other, and so there were no PLTs necessary for B/BL. After that commit we can have a 2G module range regardless of CONFIG_RANDOMIZE_BASE, and PLTs may be necessary for B/BL. We should have removed the check for !CONFIG_RANDOMIZE_BASE as part of that. > > arch/arm64/kernel/module-plts.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/arch/arm64/kernel/module-plts.c > > b/arch/arm64/kernel/module-plts.c > > index bd69a4e7cd60..21a67d52d7a0 100644 > > --- a/arch/arm64/kernel/module-plts.c > > +++ b/arch/arm64/kernel/module-plts.c > > @@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms, > > Elf64_Rela *rela, int num, > > switch (ELF64_R_TYPE(rela[i].r_info)) { > > case R_AARCH64_JUMP26: > > case R_AARCH64_CALL26: > > - if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE)) > > - break; > > - > > /* > > * We only have to consider branch targets that resolve > > * to symbols that are defined in a different section. > > I see there are two such checks (in partition_branch_plt_relas() > and in count_plts()), can you explain in more detail how you > concluded that one of them is correct but the other one is not? I believe that the one in partition_branch_plt_relas() needs to go too; that's just a minor optimization for the case where there shouldn't be any PLTs for B/BL, and it no longer holds after the module VA range rework. That was introduced in commit: d4e0340919fb9190 ("arm64/module: Optimize module load time by optimizing PLT counting") Thanks, Mark.
On 10/23/2023 5:02 PM, Mark Rutland wrote: > On Mon, Oct 23, 2023 at 10:08:33AM +0200, Arnd Bergmann wrote: >> On Mon, Oct 23, 2023, at 09:57, Maria Yu wrote: >>> Module PLT feature can be enabled even when RANDOM_BASE is disabled. >>> Break BLT entry counts of relocation types will make module plt entry >>> allocation fail and finally exec format error for even correct and plt >>> allocation available modules. > > Has an actual problem been seen in practice, or was this found by looking at > the code? I've encounter an actual problem when disalbe CONFIG_RADOM_BASE and the kernel module have the exec format error issue. > >>> >>> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> >> >> Adding Ard Biesheuvel to Cc, as he added the check in commit >> a257e02579e42 ("arm64/kernel: don't ban ADRP to work around >> Cortex-A53 erratum #843419") Thx for adding Ard. Will keep him in next patchset as well. > > I think that the actual mistake is in commit: > > 3e35d303ab7d22c4 ("arm64: module: rework module VA range selection") > > Prior to that commit, when CONFIG_RANDOMIZE_BASE=n all modules and code had to > be within 128M of each other, and so there were no PLTs necessary for B/BL. > After that commit we can have a 2G module range regardless of > CONFIG_RANDOMIZE_BASE, and PLTs may be necessary for B/BL. > > We should have removed the check for !CONFIG_RANDOMIZE_BASE as part of that. Agree with you. > >>> arch/arm64/kernel/module-plts.c | 3 --- >>> 1 file changed, 3 deletions(-) >>> >>> diff --git a/arch/arm64/kernel/module-plts.c >>> b/arch/arm64/kernel/module-plts.c >>> index bd69a4e7cd60..21a67d52d7a0 100644 >>> --- a/arch/arm64/kernel/module-plts.c >>> +++ b/arch/arm64/kernel/module-plts.c >>> @@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms, >>> Elf64_Rela *rela, int num, >>> switch (ELF64_R_TYPE(rela[i].r_info)) { >>> case R_AARCH64_JUMP26: >>> case R_AARCH64_CALL26: >>> - if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE)) >>> - break; >>> - >>> /* >>> * We only have to consider branch targets that resolve >>> * to symbols that are defined in a different section. >> >> I see there are two such checks (in partition_branch_plt_relas() >> and in count_plts()), can you explain in more detail how you >> concluded that one of them is correct but the other one is not? > > I believe that the one in partition_branch_plt_relas() needs to go too; that's > just a minor optimization for the case where there shouldn't be any PLTs for > B/BL, and it no longer holds after the module VA range rework. > > That was introduced in commit: > > d4e0340919fb9190 ("arm64/module: Optimize module load time by optimizing PLT counting") The functionality is the same from my try with plt allocated kernel modules. While the PLT entry can be dramatically reduced from ~50000 to ~500 after fix in partition_branch_plt_relas. I will send out the second patchset with fix in partition_branch_plt_relas (remove check of CONFIG_RANDOMIZE_BASE) tomorrow if no more other comments today. > > Thanks, > Mark.
diff --git a/arch/arm64/kernel/module-plts.c b/arch/arm64/kernel/module-plts.c index bd69a4e7cd60..21a67d52d7a0 100644 --- a/arch/arm64/kernel/module-plts.c +++ b/arch/arm64/kernel/module-plts.c @@ -167,9 +167,6 @@ static unsigned int count_plts(Elf64_Sym *syms, Elf64_Rela *rela, int num, switch (ELF64_R_TYPE(rela[i].r_info)) { case R_AARCH64_JUMP26: case R_AARCH64_CALL26: - if (!IS_ENABLED(CONFIG_RANDOMIZE_BASE)) - break; - /* * We only have to consider branch targets that resolve * to symbols that are defined in a different section.
Module PLT feature can be enabled even when RANDOM_BASE is disabled. Break BLT entry counts of relocation types will make module plt entry allocation fail and finally exec format error for even correct and plt allocation available modules. Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> --- arch/arm64/kernel/module-plts.c | 3 --- 1 file changed, 3 deletions(-) base-commit: 05d3ef8bba77c1b5f98d941d8b2d4aeab8118ef1