diff mbox series

[1/9] scripts/link-vmlinux.sh: Delay orphan handling warnings until final link

Message ID 20200228002244.15240-2-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series Enable orphan section warning | expand

Commit Message

Kees Cook Feb. 28, 2020, 12:22 a.m. UTC
Right now, powerpc adds "--orphan-handling=warn" to LD_FLAGS_vmlinux
to detect when there are unexpected sections getting added to the kernel
image. There is no need to report these warnings more than once, so it
can be removed until the final link stage.

This helps pave the way for other architectures to enable this, with the
end goal of enabling this warning by default for vmlinux for all
architectures.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/link-vmlinux.sh | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Nicholas Piggin March 20, 2020, 2:47 a.m. UTC | #1
Kees Cook's on February 28, 2020 10:22 am:
> Right now, powerpc adds "--orphan-handling=warn" to LD_FLAGS_vmlinux
> to detect when there are unexpected sections getting added to the kernel
> image. There is no need to report these warnings more than once, so it
> can be removed until the final link stage.
> 
> This helps pave the way for other architectures to enable this, with the
> end goal of enabling this warning by default for vmlinux for all
> architectures.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  scripts/link-vmlinux.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index 1919c311c149..416968fea685 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -255,6 +255,11 @@ info GEN modules.builtin
>  tr '\0' '\n' < modules.builtin.modinfo | sed -n 's/^[[:alnum:]:_]*\.file=//p' |
>  	tr ' ' '\n' | uniq | sed -e 's:^:kernel/:' -e 's/$/.ko/' > modules.builtin
>  
> +
> +# Do not warn about orphan sections until the final link stage.
> +saved_LDFLAGS_vmlinux="${LDFLAGS_vmlinux}"
> +LDFLAGS_vmlinux="$(echo "${LDFLAGS_vmlinux}" | sed -E 's/ --orphan-handling=warn( |$)/ /g')"
> +
>  btf_vmlinux_bin_o=""
>  if [ -n "${CONFIG_DEBUG_INFO_BTF}" ]; then
>  	if gen_btf .tmp_vmlinux.btf .btf.vmlinux.bin.o ; then
> @@ -306,6 +311,7 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
>  	fi
>  fi
>  
> +LDFLAGS_vmlinux="${saved_LDFLAGS_vmlinux}"
>  vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
>  
>  if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then

That's ugly. Why not just enable it for all archs?

Thanks,
Nick
Kees Cook March 20, 2020, 6:24 p.m. UTC | #2
On Fri, Mar 20, 2020 at 12:47:54PM +1000, Nicholas Piggin wrote:
> Kees Cook's on February 28, 2020 10:22 am:
> > Right now, powerpc adds "--orphan-handling=warn" to LD_FLAGS_vmlinux
> > to detect when there are unexpected sections getting added to the kernel
> > image. There is no need to report these warnings more than once, so it
> > can be removed until the final link stage.
> > 
> > This helps pave the way for other architectures to enable this, with the
> > end goal of enabling this warning by default for vmlinux for all
> > architectures.
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  scripts/link-vmlinux.sh | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> > index 1919c311c149..416968fea685 100755
> > --- a/scripts/link-vmlinux.sh
> > +++ b/scripts/link-vmlinux.sh
> > @@ -255,6 +255,11 @@ info GEN modules.builtin
> >  tr '\0' '\n' < modules.builtin.modinfo | sed -n 's/^[[:alnum:]:_]*\.file=//p' |
> >  	tr ' ' '\n' | uniq | sed -e 's:^:kernel/:' -e 's/$/.ko/' > modules.builtin
> >  
> > +
> > +# Do not warn about orphan sections until the final link stage.
> > +saved_LDFLAGS_vmlinux="${LDFLAGS_vmlinux}"
> > +LDFLAGS_vmlinux="$(echo "${LDFLAGS_vmlinux}" | sed -E 's/ --orphan-handling=warn( |$)/ /g')"
> > +
> >  btf_vmlinux_bin_o=""
> >  if [ -n "${CONFIG_DEBUG_INFO_BTF}" ]; then
> >  	if gen_btf .tmp_vmlinux.btf .btf.vmlinux.bin.o ; then
> > @@ -306,6 +311,7 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
> >  	fi
> >  fi
> >  
> > +LDFLAGS_vmlinux="${saved_LDFLAGS_vmlinux}"
> >  vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
> >  
> >  if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then
> 
> That's ugly. Why not just enable it for all archs?

It is ugly; I agree.

I can try to do this for all architectures, but I worry there are a
bunch I can't test. But I guess it would stand out. ;)

-Kees
Nicholas Piggin March 22, 2020, 9:16 a.m. UTC | #3
Kees Cook's on March 21, 2020 4:24 am:
> On Fri, Mar 20, 2020 at 12:47:54PM +1000, Nicholas Piggin wrote:
>> Kees Cook's on February 28, 2020 10:22 am:
>> > Right now, powerpc adds "--orphan-handling=warn" to LD_FLAGS_vmlinux
>> > to detect when there are unexpected sections getting added to the kernel
>> > image. There is no need to report these warnings more than once, so it
>> > can be removed until the final link stage.
>> > 
>> > This helps pave the way for other architectures to enable this, with the
>> > end goal of enabling this warning by default for vmlinux for all
>> > architectures.
>> > 
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> > ---
>> >  scripts/link-vmlinux.sh | 6 ++++++
>> >  1 file changed, 6 insertions(+)
>> > 
>> > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
>> > index 1919c311c149..416968fea685 100755
>> > --- a/scripts/link-vmlinux.sh
>> > +++ b/scripts/link-vmlinux.sh
>> > @@ -255,6 +255,11 @@ info GEN modules.builtin
>> >  tr '\0' '\n' < modules.builtin.modinfo | sed -n 's/^[[:alnum:]:_]*\.file=//p' |
>> >  	tr ' ' '\n' | uniq | sed -e 's:^:kernel/:' -e 's/$/.ko/' > modules.builtin
>> >  
>> > +
>> > +# Do not warn about orphan sections until the final link stage.
>> > +saved_LDFLAGS_vmlinux="${LDFLAGS_vmlinux}"
>> > +LDFLAGS_vmlinux="$(echo "${LDFLAGS_vmlinux}" | sed -E 's/ --orphan-handling=warn( |$)/ /g')"
>> > +
>> >  btf_vmlinux_bin_o=""
>> >  if [ -n "${CONFIG_DEBUG_INFO_BTF}" ]; then
>> >  	if gen_btf .tmp_vmlinux.btf .btf.vmlinux.bin.o ; then
>> > @@ -306,6 +311,7 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
>> >  	fi
>> >  fi
>> >  
>> > +LDFLAGS_vmlinux="${saved_LDFLAGS_vmlinux}"
>> >  vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
>> >  
>> >  if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then
>> 
>> That's ugly. Why not just enable it for all archs?
> 
> It is ugly; I agree.
> 
> I can try to do this for all architectures, but I worry there are a
> bunch I can't test. But I guess it would stand out. ;)

It's only warn, so it doesn't break their builds (unless there's a 
linker error on warn option I don't know about?). We had a powerpc bug 
that would have been caught with it as well, so it's not a bad idea to
get everyone using it.

I would just do it. Doesn't take much to fix.

Thanks,
Nick
Kees Cook March 22, 2020, 4 p.m. UTC | #4
On Sun, Mar 22, 2020 at 07:16:29PM +1000, Nicholas Piggin wrote:
> Kees Cook's on March 21, 2020 4:24 am:
> > On Fri, Mar 20, 2020 at 12:47:54PM +1000, Nicholas Piggin wrote:
> >> Kees Cook's on February 28, 2020 10:22 am:
> >> > Right now, powerpc adds "--orphan-handling=warn" to LD_FLAGS_vmlinux
> >> > to detect when there are unexpected sections getting added to the kernel
> >> > image. There is no need to report these warnings more than once, so it
> >> > can be removed until the final link stage.
> >> > 
> >> > This helps pave the way for other architectures to enable this, with the
> >> > end goal of enabling this warning by default for vmlinux for all
> >> > architectures.
> >> > 
> >> > Signed-off-by: Kees Cook <keescook@chromium.org>
> >> > ---
> >> >  scripts/link-vmlinux.sh | 6 ++++++
> >> >  1 file changed, 6 insertions(+)
> >> > 
> >> > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> >> > index 1919c311c149..416968fea685 100755
> >> > --- a/scripts/link-vmlinux.sh
> >> > +++ b/scripts/link-vmlinux.sh
> >> > @@ -255,6 +255,11 @@ info GEN modules.builtin
> >> >  tr '\0' '\n' < modules.builtin.modinfo | sed -n 's/^[[:alnum:]:_]*\.file=//p' |
> >> >  	tr ' ' '\n' | uniq | sed -e 's:^:kernel/:' -e 's/$/.ko/' > modules.builtin
> >> >  
> >> > +
> >> > +# Do not warn about orphan sections until the final link stage.
> >> > +saved_LDFLAGS_vmlinux="${LDFLAGS_vmlinux}"
> >> > +LDFLAGS_vmlinux="$(echo "${LDFLAGS_vmlinux}" | sed -E 's/ --orphan-handling=warn( |$)/ /g')"
> >> > +
> >> >  btf_vmlinux_bin_o=""
> >> >  if [ -n "${CONFIG_DEBUG_INFO_BTF}" ]; then
> >> >  	if gen_btf .tmp_vmlinux.btf .btf.vmlinux.bin.o ; then
> >> > @@ -306,6 +311,7 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
> >> >  	fi
> >> >  fi
> >> >  
> >> > +LDFLAGS_vmlinux="${saved_LDFLAGS_vmlinux}"
> >> >  vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
> >> >  
> >> >  if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then
> >> 
> >> That's ugly. Why not just enable it for all archs?
> > 
> > It is ugly; I agree.
> > 
> > I can try to do this for all architectures, but I worry there are a
> > bunch I can't test. But I guess it would stand out. ;)
> 
> It's only warn, so it doesn't break their builds (unless there's a 
> linker error on warn option I don't know about?). We had a powerpc bug 
> that would have been caught with it as well, so it's not a bad idea to
> get everyone using it.

Well, it's bad form to add warnings to a build. I am expected to fix any
warnings before I enable a warning flag.

> I would just do it. Doesn't take much to fix.

I will do my best on the archs I can't test. :)
diff mbox series

Patch

diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 1919c311c149..416968fea685 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -255,6 +255,11 @@  info GEN modules.builtin
 tr '\0' '\n' < modules.builtin.modinfo | sed -n 's/^[[:alnum:]:_]*\.file=//p' |
 	tr ' ' '\n' | uniq | sed -e 's:^:kernel/:' -e 's/$/.ko/' > modules.builtin
 
+
+# Do not warn about orphan sections until the final link stage.
+saved_LDFLAGS_vmlinux="${LDFLAGS_vmlinux}"
+LDFLAGS_vmlinux="$(echo "${LDFLAGS_vmlinux}" | sed -E 's/ --orphan-handling=warn( |$)/ /g')"
+
 btf_vmlinux_bin_o=""
 if [ -n "${CONFIG_DEBUG_INFO_BTF}" ]; then
 	if gen_btf .tmp_vmlinux.btf .btf.vmlinux.bin.o ; then
@@ -306,6 +311,7 @@  if [ -n "${CONFIG_KALLSYMS}" ]; then
 	fi
 fi
 
+LDFLAGS_vmlinux="${saved_LDFLAGS_vmlinux}"
 vmlinux_link vmlinux "${kallsymso}" ${btf_vmlinux_bin_o}
 
 if [ -n "${CONFIG_BUILDTIME_TABLE_SORT}" ]; then