diff mbox series

[RFC,v4,22/37] arm64: kernel: Skip validation of kuser32.o

Message ID 20220429094355.122389-23-chenzhongjin@huawei.com (mailing list archive)
State New, archived
Headers show
Series objtool: add base support for arm64 | expand

Commit Message

Chen Zhongjin April 29, 2022, 9:43 a.m. UTC
From: Raphael Gault <raphael.gault@arm.com>

kuser32 being used for compatibility, it contains a32 instructions
which are not recognised by objtool when trying to analyse arm64
object files. Thus, we add an exception to skip validation on this
particular file.

Signed-off-by: Raphael Gault <raphael.gault@arm.com>
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 arch/arm64/kernel/Makefile | 3 +++
 1 file changed, 3 insertions(+)

Comments

Peter Zijlstra April 29, 2022, 11:05 a.m. UTC | #1
On Fri, Apr 29, 2022 at 05:43:40PM +0800, Chen Zhongjin wrote:
> From: Raphael Gault <raphael.gault@arm.com>
> 
> kuser32 being used for compatibility, it contains a32 instructions
> which are not recognised by objtool when trying to analyse arm64
> object files. Thus, we add an exception to skip validation on this
> particular file.
> 
> Signed-off-by: Raphael Gault <raphael.gault@arm.com>
> Signed-off-by: Julien Thierry <jthierry@redhat.com>
> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
> ---
>  arch/arm64/kernel/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 986837d7ec82..c4f01bfe79b4 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -41,6 +41,9 @@ obj-$(CONFIG_COMPAT)			+= sys32.o signal32.o			\
>  					   sys_compat.o
>  obj-$(CONFIG_COMPAT)			+= sigreturn32.o
>  obj-$(CONFIG_KUSER_HELPERS)		+= kuser32.o
> +
> +OBJECT_FILES_NON_STANDARD_kuser32.o := y

File based skipping is depricated in the face of LTO and other link
target based objtool runs.

Please use function based blacklisting as per the previous patch.
Chen Zhongjin May 5, 2022, 3:36 a.m. UTC | #2
Hi Peter,

IIRC now the blacklist mechanisms all run on check stage, which after
decoding, but the problem of kuser32.S happens in decoding stage. Other
than that the assembly symbols in kuser32 is STT_NOTYPE and
STACK_FRAME_NON_STANDARD will throw an error for this.

OBJECT_FILES_NON_STANDARD works for the single file but as you said
after LTO it's invalid. However STACK_FRAME_NON_STANDARD doesn't work
for kuser32 case at all.

Now my strategy for undecodable instructions is: show an error message
and mark insn->ignore = true, but do not stop anything so decoding work
can going on.

To totally solve this my idea is that applying blacklist before decode.
However for this part objtool doesn't have any insn or func info, so we
should add a new blacklist just for this case...

On 2022/4/29 19:05, Peter Zijlstra wrote:
> On Fri, Apr 29, 2022 at 05:43:40PM +0800, Chen Zhongjin wrote:
>> From: Raphael Gault <raphael.gault@arm.com>
>>
>> kuser32 being used for compatibility, it contains a32 instructions
>> which are not recognised by objtool when trying to analyse arm64
>> object files. Thus, we add an exception to skip validation on this
>> particular file.
>>
>> Signed-off-by: Raphael Gault <raphael.gault@arm.com>
>> Signed-off-by: Julien Thierry <jthierry@redhat.com>
>> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
>> ---
>>  arch/arm64/kernel/Makefile | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 986837d7ec82..c4f01bfe79b4 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -41,6 +41,9 @@ obj-$(CONFIG_COMPAT)			+= sys32.o signal32.o			\
>>  					   sys_compat.o
>>  obj-$(CONFIG_COMPAT)			+= sigreturn32.o
>>  obj-$(CONFIG_KUSER_HELPERS)		+= kuser32.o
>> +
>> +OBJECT_FILES_NON_STANDARD_kuser32.o := y
> 
> File based skipping is depricated in the face of LTO and other link
> target based objtool runs.
> 
> Please use function based blacklisting as per the previous patch.
> 
> .
Peter Zijlstra May 5, 2022, 9:24 a.m. UTC | #3
On Thu, May 05, 2022 at 11:36:12AM +0800, Chen Zhongjin wrote:
> Hi Peter,
> 
> IIRC now the blacklist mechanisms all run on check stage, which after
> decoding, but the problem of kuser32.S happens in decoding stage. Other
> than that the assembly symbols in kuser32 is STT_NOTYPE and
> STACK_FRAME_NON_STANDARD will throw an error for this.
> 
> OBJECT_FILES_NON_STANDARD works for the single file but as you said
> after LTO it's invalid. However STACK_FRAME_NON_STANDARD doesn't work
> for kuser32 case at all.
> 
> Now my strategy for undecodable instructions is: show an error message
> and mark insn->ignore = true, but do not stop anything so decoding work
> can going on.
> 
> To totally solve this my idea is that applying blacklist before decode.
> However for this part objtool doesn't have any insn or func info, so we
> should add a new blacklist just for this case...

OK, so Mark explained that this is 32bit userspace (VDSO) code.

And as such there's really no point in running objtool on it. Does all
that live in it's own section? Should it?
Mark Rutland May 5, 2022, 10:56 a.m. UTC | #4
On Thu, May 05, 2022 at 11:24:48AM +0200, Peter Zijlstra wrote:
> On Thu, May 05, 2022 at 11:36:12AM +0800, Chen Zhongjin wrote:
> > Hi Peter,
> > 
> > IIRC now the blacklist mechanisms all run on check stage, which after
> > decoding, but the problem of kuser32.S happens in decoding stage. Other
> > than that the assembly symbols in kuser32 is STT_NOTYPE and
> > STACK_FRAME_NON_STANDARD will throw an error for this.
> > 
> > OBJECT_FILES_NON_STANDARD works for the single file but as you said
> > after LTO it's invalid. However STACK_FRAME_NON_STANDARD doesn't work
> > for kuser32 case at all.
> > 
> > Now my strategy for undecodable instructions is: show an error message
> > and mark insn->ignore = true, but do not stop anything so decoding work
> > can going on.
> > 
> > To totally solve this my idea is that applying blacklist before decode.
> > However for this part objtool doesn't have any insn or func info, so we
> > should add a new blacklist just for this case...
> 
> OK, so Mark explained that this is 32bit userspace (VDSO) code.
> 
> And as such there's really no point in running objtool on it. Does all
> that live in it's own section? Should it?

It's placed in .rodata by a linker script:

* The 32-bit vdso + kuser code is placed in .rodata, between the `vdso32_start`
  and `vdso32_end` symbols, as raw bytes (via .incbin).
  See arch/arm64/kernel/vdso32-wrap.S.

* The 64-bit vdso code is placed in .rodata, between the `vdso_start`
  and `vdso32` symbols, as raw bytes (via .incbin).
  See arch/arm64/kernel/vdso-wrap.S.

The objects under arch/arm64/kernel/{vdso,vdso32}/ are all userspace objects,
and from userspace's PoV the existing secrtions within those objects are
correct, so I don't think those should change.

How does x86 deal with its vdso objects?

Thanks,
Mark.
Chen Zhongjin May 6, 2022, 2:18 a.m. UTC | #5
On 2022/5/5 18:56, Mark Rutland wrote:
> On Thu, May 05, 2022 at 11:24:48AM +0200, Peter Zijlstra wrote:
>> On Thu, May 05, 2022 at 11:36:12AM +0800, Chen Zhongjin wrote:
>>> Hi Peter,
>>>
>>> IIRC now the blacklist mechanisms all run on check stage, which after
>>> decoding, but the problem of kuser32.S happens in decoding stage. Other
>>> than that the assembly symbols in kuser32 is STT_NOTYPE and
>>> STACK_FRAME_NON_STANDARD will throw an error for this.
>>>
>>> OBJECT_FILES_NON_STANDARD works for the single file but as you said
>>> after LTO it's invalid. However STACK_FRAME_NON_STANDARD doesn't work
>>> for kuser32 case at all.
>>>
>>> Now my strategy for undecodable instructions is: show an error message
>>> and mark insn->ignore = true, but do not stop anything so decoding work
>>> can going on.
>>>
>>> To totally solve this my idea is that applying blacklist before decode.
>>> However for this part objtool doesn't have any insn or func info, so we
>>> should add a new blacklist just for this case...
>>
>> OK, so Mark explained that this is 32bit userspace (VDSO) code.
>>
>> And as such there's really no point in running objtool on it. Does all
>> that live in it's own section? Should it?
> 
> It's placed in .rodata by a linker script:
> 
> * The 32-bit vdso + kuser code is placed in .rodata, between the `vdso32_start`
>   and `vdso32_end` symbols, as raw bytes (via .incbin).
>   See arch/arm64/kernel/vdso32-wrap.S.
> 
> * The 64-bit vdso code is placed in .rodata, between the `vdso_start`
>   and `vdso32` symbols, as raw bytes (via .incbin).
>   See arch/arm64/kernel/vdso-wrap.S.
> 
> The objects under arch/arm64/kernel/{vdso,vdso32}/ are all userspace objects,
> and from userspace's PoV the existing secrtions within those objects are
> correct, so I don't think those should change.
> 
> How does x86 deal with its vdso objects?
> 
> Thanks,
> Mark.
> .

However for my build kuser32.o content is in .text and there is only
`vdso` symbol in .rodata without `vdso32`. And for defconfig the
CONFIG_KUSER_HELPERS=y is on.

According to your description, it seems something wrong here?

If all 32-bit asm is placed in .rodata it won't cause problem for
objtool check.

Thanks!
Mark Rutland May 6, 2022, 10:06 a.m. UTC | #6
On Fri, May 06, 2022 at 10:18:10AM +0800, Chen Zhongjin wrote:
> On 2022/5/5 18:56, Mark Rutland wrote:
> > On Thu, May 05, 2022 at 11:24:48AM +0200, Peter Zijlstra wrote:
> >> On Thu, May 05, 2022 at 11:36:12AM +0800, Chen Zhongjin wrote:
> >>> Hi Peter,
> >>>
> >>> IIRC now the blacklist mechanisms all run on check stage, which after
> >>> decoding, but the problem of kuser32.S happens in decoding stage. Other
> >>> than that the assembly symbols in kuser32 is STT_NOTYPE and
> >>> STACK_FRAME_NON_STANDARD will throw an error for this.
> >>>
> >>> OBJECT_FILES_NON_STANDARD works for the single file but as you said
> >>> after LTO it's invalid. However STACK_FRAME_NON_STANDARD doesn't work
> >>> for kuser32 case at all.
> >>>
> >>> Now my strategy for undecodable instructions is: show an error message
> >>> and mark insn->ignore = true, but do not stop anything so decoding work
> >>> can going on.
> >>>
> >>> To totally solve this my idea is that applying blacklist before decode.
> >>> However for this part objtool doesn't have any insn or func info, so we
> >>> should add a new blacklist just for this case...
> >>
> >> OK, so Mark explained that this is 32bit userspace (VDSO) code.
> >>
> >> And as such there's really no point in running objtool on it. Does all
> >> that live in it's own section? Should it?
> > 
> > It's placed in .rodata by a linker script:
> > 
> > * The 32-bit vdso + kuser code is placed in .rodata, between the `vdso32_start`
> >   and `vdso32_end` symbols, as raw bytes (via .incbin).
> >   See arch/arm64/kernel/vdso32-wrap.S.
> > 
> > * The 64-bit vdso code is placed in .rodata, between the `vdso_start`
> >   and `vdso32` symbols, as raw bytes (via .incbin).
> >   See arch/arm64/kernel/vdso-wrap.S.
> > 
> > The objects under arch/arm64/kernel/{vdso,vdso32}/ are all userspace objects,
> > and from userspace's PoV the existing secrtions within those objects are
> > correct, so I don't think those should change.
> > 
> > How does x86 deal with its vdso objects?
> > 
> > Thanks,
> > Mark.
> > .
> 
> However for my build kuser32.o content is in .text 

We should be able to move that into .rodata; it's never executed in kernel context.

> and there is only `vdso` symbol in .rodata without `vdso32`.

That means you're not building with CROSS_COMPILE_COMPAT, and so we can't build
the 32-bit VDSO.

> And for defconfig the CONFIG_KUSER_HELPERS=y is on.

Yes.

> According to your description, it seems something wrong here?

Sorry, I was wrong about how we linked the kuser32 code.

I believe we can move that into .rodata by adding:

	.section .rodata

... to the start of that.

I think that'd be a nice cleanup to do regardless of objtool.

Thanks,
Mark.
Josh Poimboeuf May 6, 2022, 5:31 p.m. UTC | #7
On Thu, May 05, 2022 at 11:56:45AM +0100, Mark Rutland wrote:
> The objects under arch/arm64/kernel/{vdso,vdso32}/ are all userspace objects,
> and from userspace's PoV the existing secrtions within those objects are
> correct, so I don't think those should change.
> 
> How does x86 deal with its vdso objects?

On x86, we just use OBJECT_FILES_NON_STANDARD (like the up-thread patch)
to tell objtool to ignore all the vdso objects.

That should be fine for vdso, since it doesn't actually end up as text
in vmlinux.
Chen Zhongjin May 7, 2022, 6:35 a.m. UTC | #8
On 2022/5/6 18:06, Mark Rutland wrote:
>> However for my build kuser32.o content is in .text 
> 
> We should be able to move that into .rodata; it's never executed in kernel context.
> 
>> and there is only `vdso` symbol in .rodata without `vdso32`.
> 
> That means you're not building with CROSS_COMPILE_COMPAT, and so we can't build
> the 32-bit VDSO.
> 
>> And for defconfig the CONFIG_KUSER_HELPERS=y is on.
> 
> Yes.
> 
>> According to your description, it seems something wrong here?
> 
> Sorry, I was wrong about how we linked the kuser32 code.
> 
> I believe we can move that into .rodata by adding:
> 
> 	.section .rodata
> 
> ... to the start of that.

It works.

Thanks!

> I think that'd be a nice cleanup to do regardless of objtool.
> 
> Thanks,
> Mark.
> .
diff mbox series

Patch

diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 986837d7ec82..c4f01bfe79b4 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -41,6 +41,9 @@  obj-$(CONFIG_COMPAT)			+= sys32.o signal32.o			\
 					   sys_compat.o
 obj-$(CONFIG_COMPAT)			+= sigreturn32.o
 obj-$(CONFIG_KUSER_HELPERS)		+= kuser32.o
+
+OBJECT_FILES_NON_STANDARD_kuser32.o := y
+
 obj-$(CONFIG_FUNCTION_TRACER)		+= ftrace.o entry-ftrace.o
 obj-$(CONFIG_MODULES)			+= module.o
 obj-$(CONFIG_ARM64_MODULE_PLTS)		+= module-plts.o