| Message ID | 49f59a8bf2c4d95cfaa03bd3dd3c1569822ad6ba.1634190022.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix LKDTM for PPC64/IA64/PARISC | expand |
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > There are three architectures with function descriptors, try to > have common names for the address they contain in order to > refactor some functions into generic functions later. > > powerpc has 'funcaddr' > ia64 has 'ip' > parisc has 'addr' > > Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly. I would have picked 'funcaddr', but at least 'addr' is better than 'ip'! And I agree that consistency, and then making things generic is worthwhile. I grepped the latest powerpc/next for uses of 'funcaddr'. There were 5, your patch changes all 5. The series passes build tests and this patch has no checkpatch or other style concerns. On that basis: Reviewed-by: Daniel Axtens <dja@axtens.net> Kind regards, Daniel > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/include/asm/elf.h | 2 +- > arch/powerpc/include/asm/sections.h | 2 +- > arch/powerpc/kernel/module_64.c | 6 +++--- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h > index a4406714c060..bb0f278f9ed4 100644 > --- a/arch/powerpc/include/asm/elf.h > +++ b/arch/powerpc/include/asm/elf.h > @@ -178,7 +178,7 @@ void relocate(unsigned long final_address); > > /* There's actually a third entry here, but it's unused */ > struct ppc64_opd_entry { > - unsigned long funcaddr; > + unsigned long addr; > unsigned long r2; > }; > > diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h > index 6e4af4492a14..32e7035863ac 100644 > --- a/arch/powerpc/include/asm/sections.h > +++ b/arch/powerpc/include/asm/sections.h > @@ -77,7 +77,7 @@ static inline void *dereference_function_descriptor(void *ptr) > struct ppc64_opd_entry *desc = ptr; > void *p; > > - if (!get_kernel_nofault(p, (void *)&desc->funcaddr)) > + if (!get_kernel_nofault(p, (void *)&desc->addr)) > ptr = p; > return ptr; > } > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 6baa676e7cb6..82908c9be627 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr) > } > static unsigned long func_addr(unsigned long addr) > { > - return func_desc(addr).funcaddr; > + return func_desc(addr).addr; > } > static unsigned long stub_func_addr(func_desc_t func) > { > - return func.funcaddr; > + return func.addr; > } > static unsigned int local_entry_offset(const Elf64_Sym *sym) > { > @@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y) > static unsigned long get_stubs_size(const Elf64_Ehdr *hdr, > const Elf64_Shdr *sechdrs) > { > - /* One extra reloc so it's always 0-funcaddr terminated */ > + /* One extra reloc so it's always 0-addr terminated */ > unsigned long relocs = 1; > unsigned i; > > -- > 2.31.1
Le 14/10/2021 à 23:45, Daniel Axtens a écrit : > Christophe Leroy <christophe.leroy@csgroup.eu> writes: > >> There are three architectures with function descriptors, try to >> have common names for the address they contain in order to >> refactor some functions into generic functions later. >> >> powerpc has 'funcaddr' >> ia64 has 'ip' >> parisc has 'addr' >> >> Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly. > > I would have picked 'funcaddr', but at least 'addr' is better than 'ip'! > And I agree that consistency, and then making things generic is worthwhile. It's a function descriptor, there is only one address field, I don't think there is any ambiguïty here, and I prefer modifying the least impacted architectures. Changing addr to funcaddr in PARISC would result in the following changes, on an architecture I know nothing about. It's more changes than we have on powerpc. arch/parisc/include/asm/elf.h | 4 ++-- arch/parisc/kernel/kexec.c | 2 +- arch/parisc/kernel/module.c | 12 ++++++------ arch/parisc/kernel/process.c | 2 +- arch/parisc/kernel/signal.c | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) > > I grepped the latest powerpc/next for uses of 'funcaddr'. There were 5, > your patch changes all 5. > > The series passes build tests and this patch has no checkpatch or other > style concerns. > > On that basis: > Reviewed-by: Daniel Axtens <dja@axtens.net> > > Kind regards, > Daniel > >> Reviewed-by: Kees Cook <keescook@chromium.org> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/include/asm/elf.h | 2 +- >> arch/powerpc/include/asm/sections.h | 2 +- >> arch/powerpc/kernel/module_64.c | 6 +++--- >> 3 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h >> index a4406714c060..bb0f278f9ed4 100644 >> --- a/arch/powerpc/include/asm/elf.h >> +++ b/arch/powerpc/include/asm/elf.h >> @@ -178,7 +178,7 @@ void relocate(unsigned long final_address); >> >> /* There's actually a third entry here, but it's unused */ >> struct ppc64_opd_entry { >> - unsigned long funcaddr; >> + unsigned long addr; >> unsigned long r2; >> }; >> >> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h >> index 6e4af4492a14..32e7035863ac 100644 >> --- a/arch/powerpc/include/asm/sections.h >> +++ b/arch/powerpc/include/asm/sections.h >> @@ -77,7 +77,7 @@ static inline void *dereference_function_descriptor(void *ptr) >> struct ppc64_opd_entry *desc = ptr; >> void *p; >> >> - if (!get_kernel_nofault(p, (void *)&desc->funcaddr)) >> + if (!get_kernel_nofault(p, (void *)&desc->addr)) >> ptr = p; >> return ptr; >> } >> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c >> index 6baa676e7cb6..82908c9be627 100644 >> --- a/arch/powerpc/kernel/module_64.c >> +++ b/arch/powerpc/kernel/module_64.c >> @@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr) >> } >> static unsigned long func_addr(unsigned long addr) >> { >> - return func_desc(addr).funcaddr; >> + return func_desc(addr).addr; >> } >> static unsigned long stub_func_addr(func_desc_t func) >> { >> - return func.funcaddr; >> + return func.addr; >> } >> static unsigned int local_entry_offset(const Elf64_Sym *sym) >> { >> @@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y) >> static unsigned long get_stubs_size(const Elf64_Ehdr *hdr, >> const Elf64_Shdr *sechdrs) >> { >> - /* One extra reloc so it's always 0-funcaddr terminated */ >> + /* One extra reloc so it's always 0-addr terminated */ >> unsigned long relocs = 1; >> unsigned i; >> >> -- >> 2.31.1
Excerpts from Christophe Leroy's message of October 14, 2021 3:49 pm: > There are three architectures with function descriptors, try to > have common names for the address they contain in order to > refactor some functions into generic functions later. > > powerpc has 'funcaddr' > ia64 has 'ip' > parisc has 'addr' > > Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly. It is the "address of the entry point of the function" according to powerpc ELF spec, so addr seems fine. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > arch/powerpc/include/asm/elf.h | 2 +- > arch/powerpc/include/asm/sections.h | 2 +- > arch/powerpc/kernel/module_64.c | 6 +++--- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h > index a4406714c060..bb0f278f9ed4 100644 > --- a/arch/powerpc/include/asm/elf.h > +++ b/arch/powerpc/include/asm/elf.h > @@ -178,7 +178,7 @@ void relocate(unsigned long final_address); > > /* There's actually a third entry here, but it's unused */ > struct ppc64_opd_entry { > - unsigned long funcaddr; > + unsigned long addr; > unsigned long r2; > }; > > diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h > index 6e4af4492a14..32e7035863ac 100644 > --- a/arch/powerpc/include/asm/sections.h > +++ b/arch/powerpc/include/asm/sections.h > @@ -77,7 +77,7 @@ static inline void *dereference_function_descriptor(void *ptr) > struct ppc64_opd_entry *desc = ptr; > void *p; > > - if (!get_kernel_nofault(p, (void *)&desc->funcaddr)) > + if (!get_kernel_nofault(p, (void *)&desc->addr)) > ptr = p; > return ptr; > } > diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c > index 6baa676e7cb6..82908c9be627 100644 > --- a/arch/powerpc/kernel/module_64.c > +++ b/arch/powerpc/kernel/module_64.c > @@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr) > } > static unsigned long func_addr(unsigned long addr) > { > - return func_desc(addr).funcaddr; > + return func_desc(addr).addr; > } > static unsigned long stub_func_addr(func_desc_t func) > { > - return func.funcaddr; > + return func.addr; > } > static unsigned int local_entry_offset(const Elf64_Sym *sym) > { > @@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y) > static unsigned long get_stubs_size(const Elf64_Ehdr *hdr, > const Elf64_Shdr *sechdrs) > { > - /* One extra reloc so it's always 0-funcaddr terminated */ > + /* One extra reloc so it's always 0-addr terminated */ > unsigned long relocs = 1; > unsigned i; > > -- > 2.31.1 > > >
diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h index a4406714c060..bb0f278f9ed4 100644 --- a/arch/powerpc/include/asm/elf.h +++ b/arch/powerpc/include/asm/elf.h @@ -178,7 +178,7 @@ void relocate(unsigned long final_address); /* There's actually a third entry here, but it's unused */ struct ppc64_opd_entry { - unsigned long funcaddr; + unsigned long addr; unsigned long r2; }; diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h index 6e4af4492a14..32e7035863ac 100644 --- a/arch/powerpc/include/asm/sections.h +++ b/arch/powerpc/include/asm/sections.h @@ -77,7 +77,7 @@ static inline void *dereference_function_descriptor(void *ptr) struct ppc64_opd_entry *desc = ptr; void *p; - if (!get_kernel_nofault(p, (void *)&desc->funcaddr)) + if (!get_kernel_nofault(p, (void *)&desc->addr)) ptr = p; return ptr; } diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c index 6baa676e7cb6..82908c9be627 100644 --- a/arch/powerpc/kernel/module_64.c +++ b/arch/powerpc/kernel/module_64.c @@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr) } static unsigned long func_addr(unsigned long addr) { - return func_desc(addr).funcaddr; + return func_desc(addr).addr; } static unsigned long stub_func_addr(func_desc_t func) { - return func.funcaddr; + return func.addr; } static unsigned int local_entry_offset(const Elf64_Sym *sym) { @@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y) static unsigned long get_stubs_size(const Elf64_Ehdr *hdr, const Elf64_Shdr *sechdrs) { - /* One extra reloc so it's always 0-funcaddr terminated */ + /* One extra reloc so it's always 0-addr terminated */ unsigned long relocs = 1; unsigned i;