linux-kernel - Re: [PATCH v2 02/13] powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Oct 2021 06:59:31 +0200
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Daniel Axtens <dja@...ens.net>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
        Helge Deller <deller@....de>, Arnd Bergmann <arnd@...db.de>,
        Kees Cook <keescook@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-arch@...r.kernel.org, linux-ia64@...r.kernel.org,
        linux-parisc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2 02/13] powerpc: Rename 'funcaddr' to 'addr' in 'struct
 ppc64_opd_entry'



Le 14/10/2021 à 23:45, Daniel Axtens a écrit :
> Christophe Leroy <christophe.leroy@...roup.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@...ens.net>
> 
> Kind regards,
> Daniel
> 
>> Reviewed-by: Kees Cook <keescook@...omium.org>
>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.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