Linux-Next Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] powerpc: Do not consider weak unresolved symbol relocations as bad
@ 2020-01-18 17:03 Alexandre Ghiti
  2020-01-30 20:07 ` Alex Ghiti
  2020-02-04 12:01 ` Michael Ellerman
  0 siblings, 2 replies; 4+ messages in thread
From: Alexandre Ghiti @ 2020-01-18 17:03 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	linuxppc-dev, linux-kernel, Stephen Rothwell, Alexei Starovoitov,
	linux-next, Zong Li, Palmer Dabbelt
  Cc: Alexandre Ghiti

Commit 8580ac9404f6 ("bpf: Process in-kernel BTF") introduced two weak
symbols that may be unresolved at link time which result in an absolute
relocation to 0. relocs_check.sh emits the following warning:

"WARNING: 2 bad relocations
c000000001a41478 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_start
c000000001a41480 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_end"

whereas those relocations are legitimate even for a relocatable kernel
compiled with -pie option.

relocs_check.sh already excluded some weak unresolved symbols explicitly:
remove those hardcoded symbols and add some logic that parses the symbols
using nm, retrieves all the weak unresolved symbols and excludes those from
the list of the potential bad relocations.

Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
---

Changes in v2:
- Follow Stephen advice of using grep -F instead of looping over weak symbols
  using read, patch is way smaller and cleaner.
- Add missing nm in comment

 arch/powerpc/Makefile.postlink     |  4 ++--
 arch/powerpc/tools/relocs_check.sh | 20 ++++++++++++--------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/Makefile.postlink b/arch/powerpc/Makefile.postlink
index 134f12f89b92..2268396ff4bb 100644
--- a/arch/powerpc/Makefile.postlink
+++ b/arch/powerpc/Makefile.postlink
@@ -17,11 +17,11 @@ quiet_cmd_head_check = CHKHEAD $@
 quiet_cmd_relocs_check = CHKREL  $@
 ifdef CONFIG_PPC_BOOK3S_64
       cmd_relocs_check =						\
-	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$@" ; \
+	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$(NM)" "$@" ; \
 	$(BASH) $(srctree)/arch/powerpc/tools/unrel_branch_check.sh "$(OBJDUMP)" "$@"
 else
       cmd_relocs_check =						\
-	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$@"
+	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$(NM)" "$@"
 endif
 
 # `@true` prevents complaint when there is nothing to be done
diff --git a/arch/powerpc/tools/relocs_check.sh b/arch/powerpc/tools/relocs_check.sh
index 7b9fe0a567cf..014e00e74d2b 100755
--- a/arch/powerpc/tools/relocs_check.sh
+++ b/arch/powerpc/tools/relocs_check.sh
@@ -10,14 +10,21 @@
 # based on relocs_check.pl
 # Copyright © 2009 IBM Corporation
 
-if [ $# -lt 2 ]; then
-	echo "$0 [path to objdump] [path to vmlinux]" 1>&2
+if [ $# -lt 3 ]; then
+	echo "$0 [path to objdump] [path to nm] [path to vmlinux]" 1>&2
 	exit 1
 fi
 
-# Have Kbuild supply the path to objdump so we handle cross compilation.
+# Have Kbuild supply the path to objdump and nm so we handle cross compilation.
 objdump="$1"
-vmlinux="$2"
+nm="$2"
+vmlinux="$3"
+
+# Remove from the bad relocations those that match an undefined weak symbol
+# which will result in an absolute relocation to 0.
+# Weak unresolved symbols are of that form in nm output:
+# "                  w _binary__btf_vmlinux_bin_end"
+undef_weak_symbols=$($nm "$vmlinux" | awk '$1 ~ /w/ { print $2 }')
 
 bad_relocs=$(
 $objdump -R "$vmlinux" |
@@ -26,8 +33,6 @@ $objdump -R "$vmlinux" |
 	# These relocations are okay
 	# On PPC64:
 	#	R_PPC64_RELATIVE, R_PPC64_NONE
-	#	R_PPC64_ADDR64 mach_<name>
-	#	R_PPC64_ADDR64 __crc_<name>
 	# On PPC:
 	#	R_PPC_RELATIVE, R_PPC_ADDR16_HI,
 	#	R_PPC_ADDR16_HA,R_PPC_ADDR16_LO,
@@ -39,8 +44,7 @@ R_PPC_ADDR16_HI
 R_PPC_ADDR16_HA
 R_PPC_RELATIVE
 R_PPC_NONE' |
-	grep -E -v '\<R_PPC64_ADDR64[[:space:]]+mach_' |
-	grep -E -v '\<R_PPC64_ADDR64[[:space:]]+__crc_'
+	([ "$undef_weak_symbols" ] && grep -F -w -v "$undef_weak_symbols" || cat)
 )
 
 if [ -z "$bad_relocs" ]; then
-- 
2.20.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] powerpc: Do not consider weak unresolved symbol relocations as bad
  2020-01-18 17:03 [PATCH v2] powerpc: Do not consider weak unresolved symbol relocations as bad Alexandre Ghiti
@ 2020-01-30 20:07 ` Alex Ghiti
  2020-01-31  9:18   ` Michael Ellerman
  2020-02-04 12:01 ` Michael Ellerman
  1 sibling, 1 reply; 4+ messages in thread
From: Alex Ghiti @ 2020-01-30 20:07 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	linuxppc-dev, linux-kernel, Stephen Rothwell, Alexei Starovoitov,
	linux-next, Zong Li, Palmer Dabbelt

On 1/18/20 12:03 PM, Alexandre Ghiti wrote:
> Commit 8580ac9404f6 ("bpf: Process in-kernel BTF") introduced two weak
> symbols that may be unresolved at link time which result in an absolute
> relocation to 0. relocs_check.sh emits the following warning:
>
> "WARNING: 2 bad relocations
> c000000001a41478 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_start
> c000000001a41480 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_end"
>
> whereas those relocations are legitimate even for a relocatable kernel
> compiled with -pie option.
>
> relocs_check.sh already excluded some weak unresolved symbols explicitly:
> remove those hardcoded symbols and add some logic that parses the symbols
> using nm, retrieves all the weak unresolved symbols and excludes those from
> the list of the potential bad relocations.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> ---
>
> Changes in v2:
> - Follow Stephen advice of using grep -F instead of looping over weak symbols
>    using read, patch is way smaller and cleaner.
> - Add missing nm in comment
>
>   arch/powerpc/Makefile.postlink     |  4 ++--
>   arch/powerpc/tools/relocs_check.sh | 20 ++++++++++++--------
>   2 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/Makefile.postlink b/arch/powerpc/Makefile.postlink
> index 134f12f89b92..2268396ff4bb 100644
> --- a/arch/powerpc/Makefile.postlink
> +++ b/arch/powerpc/Makefile.postlink
> @@ -17,11 +17,11 @@ quiet_cmd_head_check = CHKHEAD $@
>   quiet_cmd_relocs_check = CHKREL  $@
>   ifdef CONFIG_PPC_BOOK3S_64
>         cmd_relocs_check =						\
> -	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$@" ; \
> +	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$(NM)" "$@" ; \
>   	$(BASH) $(srctree)/arch/powerpc/tools/unrel_branch_check.sh "$(OBJDUMP)" "$@"
>   else
>         cmd_relocs_check =						\
> -	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$@"
> +	$(CONFIG_SHELL) $(srctree)/arch/powerpc/tools/relocs_check.sh "$(OBJDUMP)" "$(NM)" "$@"
>   endif
>   
>   # `@true` prevents complaint when there is nothing to be done
> diff --git a/arch/powerpc/tools/relocs_check.sh b/arch/powerpc/tools/relocs_check.sh
> index 7b9fe0a567cf..014e00e74d2b 100755
> --- a/arch/powerpc/tools/relocs_check.sh
> +++ b/arch/powerpc/tools/relocs_check.sh
> @@ -10,14 +10,21 @@
>   # based on relocs_check.pl
>   # Copyright © 2009 IBM Corporation
>   
> -if [ $# -lt 2 ]; then
> -	echo "$0 [path to objdump] [path to vmlinux]" 1>&2
> +if [ $# -lt 3 ]; then
> +	echo "$0 [path to objdump] [path to nm] [path to vmlinux]" 1>&2
>   	exit 1
>   fi
>   
> -# Have Kbuild supply the path to objdump so we handle cross compilation.
> +# Have Kbuild supply the path to objdump and nm so we handle cross compilation.
>   objdump="$1"
> -vmlinux="$2"
> +nm="$2"
> +vmlinux="$3"
> +
> +# Remove from the bad relocations those that match an undefined weak symbol
> +# which will result in an absolute relocation to 0.
> +# Weak unresolved symbols are of that form in nm output:
> +# "                  w _binary__btf_vmlinux_bin_end"
> +undef_weak_symbols=$($nm "$vmlinux" | awk '$1 ~ /w/ { print $2 }')
>   
>   bad_relocs=$(
>   $objdump -R "$vmlinux" |
> @@ -26,8 +33,6 @@ $objdump -R "$vmlinux" |
>   	# These relocations are okay
>   	# On PPC64:
>   	#	R_PPC64_RELATIVE, R_PPC64_NONE
> -	#	R_PPC64_ADDR64 mach_<name>
> -	#	R_PPC64_ADDR64 __crc_<name>
>   	# On PPC:
>   	#	R_PPC_RELATIVE, R_PPC_ADDR16_HI,
>   	#	R_PPC_ADDR16_HA,R_PPC_ADDR16_LO,
> @@ -39,8 +44,7 @@ R_PPC_ADDR16_HI
>   R_PPC_ADDR16_HA
>   R_PPC_RELATIVE
>   R_PPC_NONE' |
> -	grep -E -v '\<R_PPC64_ADDR64[[:space:]]+mach_' |
> -	grep -E -v '\<R_PPC64_ADDR64[[:space:]]+__crc_'
> +	([ "$undef_weak_symbols" ] && grep -F -w -v "$undef_weak_symbols" || cat)
>   )
>   
>   if [ -z "$bad_relocs" ]; then


Hi guys,


Any thought about that ?

I do think this patch makes the whole check about absolute relocations 
clearer.
And in the future, it will avoid anyone to spend some time on those 
"bad" relocations
which actually aren't.

Thanks,

Alex


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] powerpc: Do not consider weak unresolved symbol relocations as bad
  2020-01-30 20:07 ` Alex Ghiti
@ 2020-01-31  9:18   ` Michael Ellerman
  0 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2020-01-31  9:18 UTC (permalink / raw)
  To: Alex Ghiti, Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev,
	linux-kernel, Stephen Rothwell, Alexei Starovoitov, linux-next,
	Zong Li, Palmer Dabbelt

Alex Ghiti <alex@ghiti.fr> writes:
> On 1/18/20 12:03 PM, Alexandre Ghiti wrote:
>> Commit 8580ac9404f6 ("bpf: Process in-kernel BTF") introduced two weak
>> symbols that may be unresolved at link time which result in an absolute
>> relocation to 0. relocs_check.sh emits the following warning:
>>
>> "WARNING: 2 bad relocations
>> c000000001a41478 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_start
>> c000000001a41480 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_end"
>>
>> whereas those relocations are legitimate even for a relocatable kernel
>> compiled with -pie option.
>>
>> relocs_check.sh already excluded some weak unresolved symbols explicitly:
>> remove those hardcoded symbols and add some logic that parses the symbols
>> using nm, retrieves all the weak unresolved symbols and excludes those from
>> the list of the potential bad relocations.
>>
>> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
>> ---
>>
>> Changes in v2:
>> - Follow Stephen advice of using grep -F instead of looping over weak symbols
>>    using read, patch is way smaller and cleaner.
>> - Add missing nm in comment
>>
>>   arch/powerpc/Makefile.postlink     |  4 ++--
>>   arch/powerpc/tools/relocs_check.sh | 20 ++++++++++++--------
>>   2 files changed, 14 insertions(+), 10 deletions(-)
>>
...
>
> Hi guys,
>
>
> Any thought about that ?
>
> I do think this patch makes the whole check about absolute relocations 
> clearer.
> And in the future, it will avoid anyone to spend some time on those 
> "bad" relocations
> which actually aren't.

Sorry I missed the v2. Will pick it up.

cheers

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] powerpc: Do not consider weak unresolved symbol relocations as bad
  2020-01-18 17:03 [PATCH v2] powerpc: Do not consider weak unresolved symbol relocations as bad Alexandre Ghiti
  2020-01-30 20:07 ` Alex Ghiti
@ 2020-02-04 12:01 ` Michael Ellerman
  1 sibling, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2020-02-04 12:01 UTC (permalink / raw)
  To: Alexandre Ghiti, Benjamin Herrenschmidt, Paul Mackerras,
	linuxppc-dev, linux-kernel, Stephen Rothwell, Alexei Starovoitov,
	linux-next, Zong Li, Palmer Dabbelt
  Cc: Alexandre Ghiti

On Sat, 2020-01-18 at 17:03:35 UTC, Alexandre Ghiti wrote:
> Commit 8580ac9404f6 ("bpf: Process in-kernel BTF") introduced two weak
> symbols that may be unresolved at link time which result in an absolute
> relocation to 0. relocs_check.sh emits the following warning:
> 
> "WARNING: 2 bad relocations
> c000000001a41478 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_start
> c000000001a41480 R_PPC64_ADDR64    _binary__btf_vmlinux_bin_end"
> 
> whereas those relocations are legitimate even for a relocatable kernel
> compiled with -pie option.
> 
> relocs_check.sh already excluded some weak unresolved symbols explicitly:
> remove those hardcoded symbols and add some logic that parses the symbols
> using nm, retrieves all the weak unresolved symbols and excludes those from
> the list of the potential bad relocations.
> 
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/43e76cd368fbb67e767da5363ffeaa3989993c8c

cheers

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-18 17:03 [PATCH v2] powerpc: Do not consider weak unresolved symbol relocations as bad Alexandre Ghiti
2020-01-30 20:07 ` Alex Ghiti
2020-01-31  9:18   ` Michael Ellerman
2020-02-04 12:01 ` Michael Ellerman

Linux-Next Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-next/0 linux-next/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-next linux-next/ https://lore.kernel.org/linux-next \
		linux-next@vger.kernel.org
	public-inbox-index linux-next

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-next


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git