Re: [PATCH v4 00/45] x86: Kernel IBT

Re: [PATCH v4 00/45] x86: Kernel IBT

[Alexei Starovoitov]

On Tue, Mar 08, 2022 at 04:30:11PM +0100, Peter Zijlstra wrote:
> Hopefully last posting...
> 
> Since last time:
> 
>  - updated the ftrace_location() patch (naveen, rostedt)
>  - added a few comments and clarifications (bpetkov)
>  - disable jump-tables (joao)
>  - verified clang-14-rc2 works
>  - fixed a whole bunch of objtool unreachable insn issue
>  - picked up a few more tags
> 
> Patches go on top of tip/master + arm64/for-next/linkage. Also available here:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/wip.ibt

I've tried to test it.
Applied the first 23 patches, since patch 24 failed to apply to bpf and bpf-next trees.
selftest/bpf/test_progs
shows that all bpf trampoline tests are failing and
eventually the kernel is crashing:
[   53.040582] RIP: 0010:do_init_module+0x9/0x6f0
[   53.052044] Call Trace:
[   53.052319]  <TASK>
[   53.052559]  bpf_trampoline_6442471381_0+0x32/0x1000
[   53.053117]  do_init_module+0x5/0x6f0
[   53.053550]  load_module+0x77c0/0x9c00

I havne't had time to debug what's going on.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 08, 2022 at 12:00:52PM -0800, Alexei Starovoitov wrote:
> On Tue, Mar 08, 2022 at 04:30:11PM +0100, Peter Zijlstra wrote:
> > Hopefully last posting...
> > 
> > Since last time:
> > 
> >  - updated the ftrace_location() patch (naveen, rostedt)
> >  - added a few comments and clarifications (bpetkov)
> >  - disable jump-tables (joao)
> >  - verified clang-14-rc2 works
> >  - fixed a whole bunch of objtool unreachable insn issue
> >  - picked up a few more tags
> > 
> > Patches go on top of tip/master + arm64/for-next/linkage. Also available here:
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/wip.ibt
> 
> I've tried to test it.

I could cleanly do:

git checkout tip/master
git merge bpf-next/master
git merge queue/x86/wip.ibt

You want me to push out that result somewhere?

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 08, 2022 at 11:01:04PM +0100, Peter Zijlstra wrote:
> On Tue, Mar 08, 2022 at 12:00:52PM -0800, Alexei Starovoitov wrote:
> > On Tue, Mar 08, 2022 at 04:30:11PM +0100, Peter Zijlstra wrote:
> > > Hopefully last posting...
> > > 
> > > Since last time:
> > > 
> > >  - updated the ftrace_location() patch (naveen, rostedt)
> > >  - added a few comments and clarifications (bpetkov)
> > >  - disable jump-tables (joao)
> > >  - verified clang-14-rc2 works
> > >  - fixed a whole bunch of objtool unreachable insn issue
> > >  - picked up a few more tags
> > > 
> > > Patches go on top of tip/master + arm64/for-next/linkage. Also available here:
> > > 
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/wip.ibt
> > 
> > I've tried to test it.
> 
> I could cleanly do:
> 
> git checkout tip/master
> git merge bpf-next/master
> git merge queue/x86/wip.ibt
> 
> You want me to push out that result somewhere?

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt

includes bpf-next/master.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 08, 2022 at 11:32:37PM +0100, Peter Zijlstra wrote:
> On Tue, Mar 08, 2022 at 11:01:04PM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 08, 2022 at 12:00:52PM -0800, Alexei Starovoitov wrote:
> > > On Tue, Mar 08, 2022 at 04:30:11PM +0100, Peter Zijlstra wrote:
> > > > Hopefully last posting...
> > > > 
> > > > Since last time:
> > > > 
> > > >  - updated the ftrace_location() patch (naveen, rostedt)
> > > >  - added a few comments and clarifications (bpetkov)
> > > >  - disable jump-tables (joao)
> > > >  - verified clang-14-rc2 works
> > > >  - fixed a whole bunch of objtool unreachable insn issue
> > > >  - picked up a few more tags
> > > > 
> > > > Patches go on top of tip/master + arm64/for-next/linkage. Also available here:
> > > > 
> > > >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/wip.ibt
> > > 
> > > I've tried to test it.
> > 
> > I could cleanly do:
> > 
> > git checkout tip/master
> > git merge bpf-next/master
> > git merge queue/x86/wip.ibt
> > 
> > You want me to push out that result somewhere?
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt
> 
> includes bpf-next/master.

I just managed to run bpf selftests with that kernel on a tigerlake
platform.  Seems to still work.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Alexei Starovoitov]

On Wed, Mar 09, 2022 at 02:02:20AM +0100, Peter Zijlstra wrote:
> On Tue, Mar 08, 2022 at 11:32:37PM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 08, 2022 at 11:01:04PM +0100, Peter Zijlstra wrote:
> > > On Tue, Mar 08, 2022 at 12:00:52PM -0800, Alexei Starovoitov wrote:
> > > > On Tue, Mar 08, 2022 at 04:30:11PM +0100, Peter Zijlstra wrote:
> > > > > Hopefully last posting...
> > > > > 
> > > > > Since last time:
> > > > > 
> > > > >  - updated the ftrace_location() patch (naveen, rostedt)
> > > > >  - added a few comments and clarifications (bpetkov)
> > > > >  - disable jump-tables (joao)
> > > > >  - verified clang-14-rc2 works
> > > > >  - fixed a whole bunch of objtool unreachable insn issue
> > > > >  - picked up a few more tags
> > > > > 
> > > > > Patches go on top of tip/master + arm64/for-next/linkage. Also available here:
> > > > > 
> > > > >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/wip.ibt
> > > > 
> > > > I've tried to test it.
> > > 
> > > I could cleanly do:
> > > 
> > > git checkout tip/master
> > > git merge bpf-next/master
> > > git merge queue/x86/wip.ibt
> > > 
> > > You want me to push out that result somewhere?
> > 
> >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt
> > 
> > includes bpf-next/master.
> 
> I just managed to run bpf selftests with that kernel on a tigerlake
> platform.  Seems to still work.

Pulled above and it got even worse.
With kasan and lockdep during qemu boot I see:
[    1.147498] rcu_scheduler_active = 1, debug_locks = 1
[    1.147498] 2 locks held by kthreadd/2:
[    1.147498]  #0: ffff888100362b80 (&p->pi_lock){....}-{2:2}, at: task_rq_lock+0x71/0x380
[    1.147498]  #1: ffff8881f6a3a218 (&rq->__lock){-...}-{2:2}, at: raw_spin_rq_lock_nested+0x2b/0x40
[    1.147498]
[    1.147498] stack backtrace:
[    1.147498] CPU: 0 PID: 2 Comm: kthreadd Not tainted 5.17.0-rc7-02289-gc958c6aae879 #1
[    1.147498] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
[    1.147498] Call Trace:
[    1.147498]  <TASK>
[    1.147498]  dump_stack_lvl+0x48/0x5b
[    1.147498]  cpuacct_charge+0x2b3/0x390
[    1.147498]  update_curr+0x33e/0x7d0
[    1.147498]  dequeue_entity+0x28/0xdf0
[    1.147498]  ? rcu_read_lock_bh_held+0xa0/0xa0
[    1.147498]  dequeue_task_fair+0x1fa/0xd60
[    1.147498]  __do_set_cpus_allowed+0x253/0x620
[    1.147498]  __set_cpus_allowed_ptr_locked+0x25f/0x450
[    1.147498]  __set_cpus_allowed_ptr+0x7c/0xa0
[    1.147498]  ? __set_cpus_allowed_ptr_locked+0x450/0x450
[    1.147498]  ? _raw_spin_unlock_irqrestore+0x34/0x60
[    1.147498]  ? lockdep_hardirqs_on+0x7d/0x100
[    1.147498]  kthreadd+0x48/0x610
[    1.147498]  ? _raw_spin_unlock_irq+0x28/0x50
[    1.147498]  ? kthread_is_per_cpu+0xc0/0xc0
[    1.147498]  ret_from_fork+0x1f/0x30

[    6.698206] ======================================================
[    6.698209] WARNING: possible circular locking dependency detected
[    6.698211] 5.17.0-rc7-02289-gc958c6aae879 #1 Not tainted
[    6.698213] ------------------------------------------------------
[    6.698214] scsi_eh_1/401 is trying to acquire lock:
[    6.698216] ffff888100360900 (&p->pi_lock){-.-.}-{2:2}, at: try_to_wake_up+0xb6/0x1570
[    6.698241]
[    6.698241] but task is already holding lock:
[    6.698241] ffffffff854439f8 ((console_sem).lock){-...}-{2:2}, at: up+0x1b/0xe0
[    6.698253]
[    6.698253] which lock already depends on the new lock.
[    6.698253]
[    6.698254]
[    6.698254] the existing dependency chain (in reverse order) is:
[    6.698255]
[    6.698255] -> #2 ((console_sem).lock){-...}-{2:2}:
[    6.698259]        _raw_spin_lock_irqsave+0x3c/0x60
[    6.698270]        down_trylock+0x17/0x70
[    6.698274]        __down_trylock_console_sem+0x23/0x90
[    6.698278]        console_trylock+0x17/0x70
[    6.698281]        vprintk_emit+0x72/0x290
[    6.698288]        _printk+0x9a/0xb4
[    6.698296]        lockdep_rcu_suspicious+0x60/0x158
[    6.698299]        cpuacct_charge+0x2b3/0x390
[    6.698302]        update_curr+0x33e/0x7d0
[    6.698306]        dequeue_entity+0x28/0xdf0
[    6.698308]        dequeue_task_fair+0x1fa/0xd60

Most of the time it hangs during the boot.
I'm using gcc 8.5 and qemu -smp 8

With qemu -smp 1 it luckly boots.
Then I run test_progs and see:
Summary: 215/1115 PASSED, 4 SKIPPED, 18 FAILED
All trampoline tests fail.
Here is one:
$ test_progs -t fentry
test_fentry_fexit:PASS:fentry_skel_load 0 nsec
test_fentry_fexit:PASS:fexit_skel_load 0 nsec
test_fentry_fexit:PASS:fentry_attach 0 nsec
test_fentry_fexit:FAIL:fexit_attach unexpected error: -1 (errno 19)
#54 fentry_fexit:FAIL

or

./test_progs -t xdp_bpf
test_xdp_bpf2bpf:PASS:test_xdp__open_and_load 0 nsec
test_xdp_bpf2bpf:PASS:test_xdp_bpf2bpf__open 0 nsec
test_xdp_bpf2bpf:PASS:test_xdp_bpf2bpf__load 0 nsec
libbpf: prog 'trace_on_entry': failed to attach: Device or resource busy
libbpf: prog 'trace_on_entry': failed to auto-attach: -16
test_xdp_bpf2bpf:FAIL:test_xdp_bpf2bpf__attach unexpected error: -16 (errno 16)
#225 xdp_bpf2bpf:FAIL

Re: [PATCH v4 00/45] x86: Kernel IBT

[Nick Desaulniers]

On Tue, Mar 8, 2022 at 2:32 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Mar 08, 2022 at 11:01:04PM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 08, 2022 at 12:00:52PM -0800, Alexei Starovoitov wrote:
> > > On Tue, Mar 08, 2022 at 04:30:11PM +0100, Peter Zijlstra wrote:
> > > > Hopefully last posting...
> > > >
> > > > Since last time:
> > > >  - verified clang-14-rc2 works
> > > >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/wip.ibt

I observed the following error when building with
CONFIG_LTO_CLANG_FULL=y enabled:

ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
already defined
        ibt_selftest_ip:
        ^

Seems to come from
commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")

Commenting out the label in the inline asm, I then observed:
vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
callable instruction with modified stack frame
vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
mismatch: cfa1=4+64 cfa2=4+8
These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
the ibt_selftest_ip label).

Otherwise defconfig and CONFIG_LTO_CLANG_THIN=y both built and booted
in a vm WITHOUT IBT support.

Any idea what's the status of IBT emulation in QEMU, and if it exists,
what's the necessary `-cpu` flag to enable it?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:

> I observed the following error when building with
> CONFIG_LTO_CLANG_FULL=y enabled:
> 
> ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> already defined
>         ibt_selftest_ip:
>         ^
> 
> Seems to come from
> commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> 
> Commenting out the label in the inline asm, I then observed:
> vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> callable instruction with modified stack frame
> vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> mismatch: cfa1=4+64 cfa2=4+8
> These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> the ibt_selftest_ip label).

Urgh.. I'm thikning this is a clang bug :/

The code in question is:


void ibt_selftest_ip(void); /* code label defined in asm below */

DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
{
	/* ... */

	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
		regs->ax = 0;
		return;
	}

	/* ... */
}

bool ibt_selftest(void)
{
	unsigned long ret;

	asm ("	lea ibt_selftest_ip(%%rip), %%rax\n\t"
	     ANNOTATE_RETPOLINE_SAFE
	     "	jmp *%%rax\n\t"
	     "ibt_selftest_ip:\n\t"
	     UNWIND_HINT_FUNC
	     ANNOTATE_NOENDBR
	     "	nop\n\t"

	     : "=a" (ret) : : "memory");

	return !ret;
}

There is only a single definition of that symbol, the one in the asm.
The other is a declaration, which is used in the exception handler to
compare against regs->ip.

So what this code does is trigger an explicit #CP and special case that
in the handler. For that the handler needs to know the special IP that
will trigger the failure, this is cummunicated with that symbol.

> Otherwise defconfig and CONFIG_LTO_CLANG_THIN=y both built and booted
> in a vm WITHOUT IBT support.
> 
> Any idea what's the status of IBT emulation in QEMU, and if it exists,
> what's the necessary `-cpu` flag to enable it?

I have a very ugly kvm patch that goes with a very ugly qemu patch to
make it work. I would very much not recommend those getting merged.

Someone with some actual kvm/qemu foo should do one. The complicating
factor is that IA32_S_CET also contains SHSTK enable bits, so a straight
passthrough like I use relies on the guest never setting those bits or
keeping the pieces. It either needs to filter the MSR or implement the
full CET mess.

RE: [PATCH v4 00/45] x86: Kernel IBT

[David Laight]

From: Peter Zijlstra
> Sent: 10 March 2022 09:05
> 
> On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:
> 
> > I observed the following error when building with
> > CONFIG_LTO_CLANG_FULL=y enabled:
> >
> > ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> > already defined
> >         ibt_selftest_ip:
> >         ^
> >
> > Seems to come from
> > commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> >
> > Commenting out the label in the inline asm, I then observed:
> > vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> > callable instruction with modified stack frame
> > vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> > mismatch: cfa1=4+64 cfa2=4+8
> > These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> > disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> > the ibt_selftest_ip label).
> 
> Urgh.. I'm thikning this is a clang bug :/
> 
> The code in question is:
> 
> 
> void ibt_selftest_ip(void); /* code label defined in asm below */
> 
> DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> {
> 	/* ... */
> 
> 	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
> 		regs->ax = 0;
> 		return;
> 	}
> 
> 	/* ... */
> }
> 
> bool ibt_selftest(void)
> {
> 	unsigned long ret;
> 
> 	asm ("	lea ibt_selftest_ip(%%rip), %%rax\n\t"
> 	     ANNOTATE_RETPOLINE_SAFE
> 	     "	jmp *%%rax\n\t"
> 	     "ibt_selftest_ip:\n\t"
> 	     UNWIND_HINT_FUNC
> 	     ANNOTATE_NOENDBR
> 	     "	nop\n\t"
> 
> 	     : "=a" (ret) : : "memory");
> 
> 	return !ret;
> }
> 
> There is only a single definition of that symbol, the one in the asm.
> The other is a declaration, which is used in the exception handler to
> compare against regs->ip.

LTO has probably inlined it twice.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Wed, Mar 09, 2022 at 11:09:17AM -0800, Alexei Starovoitov wrote:
> Pulled above and it got even worse.
> With kasan and lockdep during qemu boot I see:
> [    1.147498] rcu_scheduler_active = 1, debug_locks = 1
> [    1.147498] 2 locks held by kthreadd/2:
> [    1.147498]  #0: ffff888100362b80 (&p->pi_lock){....}-{2:2}, at: task_rq_lock+0x71/0x380
> [    1.147498]  #1: ffff8881f6a3a218 (&rq->__lock){-...}-{2:2}, at: raw_spin_rq_lock_nested+0x2b/0x40
> [    1.147498]
> [    1.147498] stack backtrace:
> [    1.147498] CPU: 0 PID: 2 Comm: kthreadd Not tainted 5.17.0-rc7-02289-gc958c6aae879 #1
> [    1.147498] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> [    1.147498] Call Trace:
> [    1.147498]  <TASK>
> [    1.147498]  dump_stack_lvl+0x48/0x5b
> [    1.147498]  cpuacct_charge+0x2b3/0x390
> [    1.147498]  update_curr+0x33e/0x7d0
> [    1.147498]  dequeue_entity+0x28/0xdf0
> [    1.147498]  ? rcu_read_lock_bh_held+0xa0/0xa0
> [    1.147498]  dequeue_task_fair+0x1fa/0xd60
> [    1.147498]  __do_set_cpus_allowed+0x253/0x620
> [    1.147498]  __set_cpus_allowed_ptr_locked+0x25f/0x450
> [    1.147498]  __set_cpus_allowed_ptr+0x7c/0xa0
> [    1.147498]  ? __set_cpus_allowed_ptr_locked+0x450/0x450
> [    1.147498]  ? _raw_spin_unlock_irqrestore+0x34/0x60
> [    1.147498]  ? lockdep_hardirqs_on+0x7d/0x100
> [    1.147498]  kthreadd+0x48/0x610
> [    1.147498]  ? _raw_spin_unlock_irq+0x28/0x50
> [    1.147498]  ? kthread_is_per_cpu+0xc0/0xc0
> [    1.147498]  ret_from_fork+0x1f/0x30

Yeah, sorry about that, currently arguing with Paul about that one.
Should go away if you disable the RCU lockdep thing. The warning itself
is a false positive and more harmful than anything else due to it
generating a possible printk deadlock.

> Most of the time it hangs during the boot.
> I'm using gcc 8.5 and qemu -smp 8

> With qemu -smp 1 it luckly boots.
> Then I run test_progs and see:
> Summary: 215/1115 PASSED, 4 SKIPPED, 18 FAILED
> All trampoline tests fail.
> Here is one:
> $ test_progs -t fentry
> test_fentry_fexit:PASS:fentry_skel_load 0 nsec
> test_fentry_fexit:PASS:fexit_skel_load 0 nsec
> test_fentry_fexit:PASS:fentry_attach 0 nsec
> test_fentry_fexit:FAIL:fexit_attach unexpected error: -1 (errno 19)
> #54 fentry_fexit:FAIL
> 
> or
> 
> ./test_progs -t xdp_bpf
> test_xdp_bpf2bpf:PASS:test_xdp__open_and_load 0 nsec
> test_xdp_bpf2bpf:PASS:test_xdp_bpf2bpf__open 0 nsec
> test_xdp_bpf2bpf:PASS:test_xdp_bpf2bpf__load 0 nsec
> libbpf: prog 'trace_on_entry': failed to attach: Device or resource busy
> libbpf: prog 'trace_on_entry': failed to auto-attach: -16
> test_xdp_bpf2bpf:FAIL:test_xdp_bpf2bpf__attach unexpected error: -16 (errno 16)
> #225 xdp_bpf2bpf:FAIL

Urgh.. I totally missed that in flood of output. Let me go try and
figure out what's happening there.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Thu, Mar 10, 2022 at 09:22:59AM +0000, David Laight wrote:
> From: Peter Zijlstra
> > Sent: 10 March 2022 09:05
> > 
> > On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:
> > 
> > > I observed the following error when building with
> > > CONFIG_LTO_CLANG_FULL=y enabled:
> > >
> > > ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> > > already defined
> > >         ibt_selftest_ip:
> > >         ^
> > >
> > > Seems to come from
> > > commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> > >
> > > Commenting out the label in the inline asm, I then observed:
> > > vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> > > callable instruction with modified stack frame
> > > vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> > > mismatch: cfa1=4+64 cfa2=4+8
> > > These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> > > disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> > > the ibt_selftest_ip label).
> > 
> > Urgh.. I'm thikning this is a clang bug :/
> > 
> > The code in question is:
> > 
> > 
> > void ibt_selftest_ip(void); /* code label defined in asm below */
> > 
> > DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
> > {
> > 	/* ... */
> > 
> > 	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
> > 		regs->ax = 0;
> > 		return;
> > 	}
> > 
> > 	/* ... */
> > }
> > 
> > bool ibt_selftest(void)
> > {
> > 	unsigned long ret;
> > 
> > 	asm ("	lea ibt_selftest_ip(%%rip), %%rax\n\t"
> > 	     ANNOTATE_RETPOLINE_SAFE
> > 	     "	jmp *%%rax\n\t"
> > 	     "ibt_selftest_ip:\n\t"
> > 	     UNWIND_HINT_FUNC
> > 	     ANNOTATE_NOENDBR
> > 	     "	nop\n\t"
> > 
> > 	     : "=a" (ret) : : "memory");
> > 
> > 	return !ret;
> > }
> > 
> > There is only a single definition of that symbol, the one in the asm.
> > The other is a declaration, which is used in the exception handler to
> > compare against regs->ip.
> 
> LTO has probably inlined it twice.

Indeed, adding noinline to ibt_selftest() makes it work.


---
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index d8bbc705efe5..0c737cc31ee5 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -781,7 +781,8 @@ int3_exception_notify(struct notifier_block *self, unsigned long val, void *data
 	return NOTIFY_STOP;
 }
 
-static void __init int3_selftest(void)
+/* Must be noinline to ensure uniqueness of int3_selftest_ip. */
+static noinline void __init int3_selftest(void)
 {
 	static __initdata struct notifier_block int3_exception_nb = {
 		.notifier_call	= int3_exception_notify,
@@ -794,9 +795,8 @@ static void __init int3_selftest(void)
 	/*
 	 * Basically: int3_magic(&val); but really complicated :-)
 	 *
-	 * Stick the address of the INT3 instruction into int3_selftest_ip,
-	 * then trigger the INT3, padded with NOPs to match a CALL instruction
-	 * length.
+	 * INT3 padded with NOP to CALL_INSN_SIZE. The int3_exception_nb
+	 * notifier above will emulate CALL for us.
 	 */
 	asm volatile ("int3_selftest_ip:\n\t"
 		      ANNOTATE_NOENDBR
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 837cc3c7d4f4..fb89a2f1011f 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -214,7 +214,7 @@ DEFINE_IDTENTRY(exc_overflow)
 
 static __ro_after_init bool ibt_fatal = true;
 
-void ibt_selftest_ip(void); /* code label defined in asm below */
+extern void ibt_selftest_ip(void); /* code label defined in asm below */
 
 enum cp_error_code {
 	CP_EC        = (1 << 15) - 1,
@@ -238,7 +238,7 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
 	if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) != CP_ENDBR))
 		return;
 
-	if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
+	if (unlikely(regs->ip == (unsigned long)&ibt_selftest_ip)) {
 		regs->ax = 0;
 		return;
 	}
@@ -252,7 +252,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
 	BUG();
 }
 
-bool ibt_selftest(void)
+/* Must be noinline to ensure uniqueness of ibt_selftest_ip. */
+noinline bool ibt_selftest(void)
 {
 	unsigned long ret;
 

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Thu, Mar 10, 2022 at 10:35:32AM +0100, Peter Zijlstra wrote:

> > $ test_progs -t fentry
> > test_fentry_fexit:PASS:fentry_skel_load 0 nsec
> > test_fentry_fexit:PASS:fexit_skel_load 0 nsec
> > test_fentry_fexit:PASS:fentry_attach 0 nsec
> > test_fentry_fexit:FAIL:fexit_attach unexpected error: -1 (errno 19)
> > #54 fentry_fexit:FAIL

This seems to cure the above. fexit_bpf2bpf is still failing, I'll dig
into that after lunch.

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index acb50fb7ed2d..2d86d3c09d64 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5354,6 +5381,11 @@ int modify_ftrace_direct(unsigned long ip,
 	mutex_lock(&direct_mutex);
 
 	mutex_lock(&ftrace_lock);
+
+	ip = ftrace_location(ip);
+	if (!ip)
+		goto out_unlock;
+
 	entry = find_direct_entry(&ip, &rec);
 	if (!entry)
 		goto out_unlock;

Re: [PATCH v4 00/45] x86: Kernel IBT

[Steven Rostedt]

On Thu, 10 Mar 2022 14:47:18 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index acb50fb7ed2d..2d86d3c09d64 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5354,6 +5381,11 @@ int modify_ftrace_direct(unsigned long ip,
>  	mutex_lock(&direct_mutex);
>  
>  	mutex_lock(&ftrace_lock);
> +
> +	ip = ftrace_location(ip);
> +	if (!ip)
> +		goto out_unlock;
> +

Perhaps this should go into find_direct_entry() instead, as I think you are
adding it before all the find_direct_entry() callers.

And find_direct_entry will update the ip.

-- Steve

>  	entry = find_direct_entry(&ip, &rec);
>  	if (!entry)
>  		goto out_unlock;

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Thu, Mar 10, 2022 at 02:47:18PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 10, 2022 at 10:35:32AM +0100, Peter Zijlstra wrote:
> 
> > > $ test_progs -t fentry
> > > test_fentry_fexit:PASS:fentry_skel_load 0 nsec
> > > test_fentry_fexit:PASS:fexit_skel_load 0 nsec
> > > test_fentry_fexit:PASS:fentry_attach 0 nsec
> > > test_fentry_fexit:FAIL:fexit_attach unexpected error: -1 (errno 19)
> > > #54 fentry_fexit:FAIL
> 
> This seems to cure the above. fexit_bpf2bpf is still failing, I'll dig
> into that after lunch.
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index acb50fb7ed2d..2d86d3c09d64 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5354,6 +5381,11 @@ int modify_ftrace_direct(unsigned long ip,
>  	mutex_lock(&direct_mutex);
>  
>  	mutex_lock(&ftrace_lock);
> +
> +	ip = ftrace_location(ip);
> +	if (!ip)
> +		goto out_unlock;
> +
>  	entry = find_direct_entry(&ip, &rec);
>  	if (!entry)
>  		goto out_unlock;

This seems to cure most of the rest. I'm still seeing one failure:

libbpf: prog 'connect_v4_prog': BPF program load failed: Invalid argument
libbpf: failed to load program 'connect_v4_prog'
libbpf: failed to load object './connect4_prog.o'
test_fexit_bpf2bpf_common:FAIL:tgt_prog_load unexpected error: -22 (errno 22)
#48/4 fexit_bpf2bpf/func_replace_verify:FAIL

but the rest seems to be good again.

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 2b1e266ff95c..3fecfe8c4429 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -384,6 +395,13 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 		/* BPF poking in modules is not supported */
 		return -EINVAL;
 
+	/*
+	 * See emit_prologue(), for IBT builds the trampoline hook is preceded
+	 * with an ENDBR instruction.
+	 */
+	if (is_endbr(*(u32 *)ip))
+		ip += ENDBR_INSN_SIZE;
+
 	return __bpf_arch_text_poke(ip, t, old_addr, new_addr, true);
 }
 

Re: [PATCH v4 00/45] x86: Kernel IBT

[Nick Desaulniers]

On Thu, Mar 10, 2022 at 2:16 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Mar 10, 2022 at 09:22:59AM +0000, David Laight wrote:
> > From: Peter Zijlstra
> > > Sent: 10 March 2022 09:05
> > >
> > > On Wed, Mar 09, 2022 at 04:30:28PM -0800, Nick Desaulniers wrote:
> > >
> > > > I observed the following error when building with
> > > > CONFIG_LTO_CLANG_FULL=y enabled:
> > > >
> > > > ld.lld: error: ld-temp.o <inline asm>:7:2: symbol 'ibt_selftest_ip' is
> > > > already defined
> > > >         ibt_selftest_ip:
> > > >         ^
> > > >
> > > > Seems to come from
> > > > commit a802350ba65a ("x86/ibt: Add IBT feature, MSR and #CP handling")
> > > >
> > > > Commenting out the label in the inline asm, I then observed:
> > > > vmlinux.o: warning: objtool: identify_cpu()+0x6d0: sibling call from
> > > > callable instruction with modified stack frame
> > > > vmlinux.o: warning: objtool: identify_cpu()+0x6e0: stack state
> > > > mismatch: cfa1=4+64 cfa2=4+8
> > > > These seemed to disappear when I kept CONFIG_LTO_CLANG_FULL=y but then
> > > > disabled CONFIG_X86_KERNEL_IBT. (perhaps due to the way I hacked out
> > > > the ibt_selftest_ip label).
> > >
> > LTO has probably inlined it twice.
>
> Indeed, adding noinline to ibt_selftest() makes it work.

Yep, that LGTM. If you end up sticking that as a patch on top:

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

For the kernel IBT series @ v4 plus this diff:

Tested-by: Nick Desaulniers <ndesaulniers@google.com> # llvm build, non-IBT boot

>
>
> ---
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index d8bbc705efe5..0c737cc31ee5 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -781,7 +781,8 @@ int3_exception_notify(struct notifier_block *self, unsigned long val, void *data
>         return NOTIFY_STOP;
>  }
>
> -static void __init int3_selftest(void)
> +/* Must be noinline to ensure uniqueness of int3_selftest_ip. */
> +static noinline void __init int3_selftest(void)
>  {
>         static __initdata struct notifier_block int3_exception_nb = {
>                 .notifier_call  = int3_exception_notify,
> @@ -794,9 +795,8 @@ static void __init int3_selftest(void)
>         /*
>          * Basically: int3_magic(&val); but really complicated :-)
>          *
> -        * Stick the address of the INT3 instruction into int3_selftest_ip,
> -        * then trigger the INT3, padded with NOPs to match a CALL instruction
> -        * length.
> +        * INT3 padded with NOP to CALL_INSN_SIZE. The int3_exception_nb
> +        * notifier above will emulate CALL for us.
>          */
>         asm volatile ("int3_selftest_ip:\n\t"
>                       ANNOTATE_NOENDBR
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 837cc3c7d4f4..fb89a2f1011f 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -214,7 +214,7 @@ DEFINE_IDTENTRY(exc_overflow)
>
>  static __ro_after_init bool ibt_fatal = true;
>
> -void ibt_selftest_ip(void); /* code label defined in asm below */
> +extern void ibt_selftest_ip(void); /* code label defined in asm below */
>
>  enum cp_error_code {
>         CP_EC        = (1 << 15) - 1,
> @@ -238,7 +238,7 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
>         if (WARN_ON_ONCE(user_mode(regs) || (error_code & CP_EC) != CP_ENDBR))
>                 return;
>
> -       if (unlikely(regs->ip == (unsigned long)ibt_selftest_ip)) {
> +       if (unlikely(regs->ip == (unsigned long)&ibt_selftest_ip)) {

(Though adding the address of operator & to the function name in the
comparisons isn't strictly necessary; functions used in expressions
"decay" into function pointers; I guess the standard calls these
"function designators." I see that's been added to be consistent
between the two...See 6.3.2.1.4 of
http://open-std.org/jtc1/sc22/wg14/www/docs/n2731.pdf pdf page
62/printed page 46.)

>                 regs->ax = 0;
>                 return;
>         }
> @@ -252,7 +252,8 @@ DEFINE_IDTENTRY_ERRORCODE(exc_control_protection)
>         BUG();
>  }
>
> -bool ibt_selftest(void)
> +/* Must be noinline to ensure uniqueness of ibt_selftest_ip. */
> +noinline bool ibt_selftest(void)
>  {
>         unsigned long ret;
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Thu, Mar 10, 2022 at 05:29:11PM +0100, Peter Zijlstra wrote:

> This seems to cure most of the rest. I'm still seeing one failure:
> 
> libbpf: prog 'connect_v4_prog': BPF program load failed: Invalid argument
> libbpf: failed to load program 'connect_v4_prog'
> libbpf: failed to load object './connect4_prog.o'
> test_fexit_bpf2bpf_common:FAIL:tgt_prog_load unexpected error: -22 (errno 22)
> #48/4 fexit_bpf2bpf/func_replace_verify:FAIL


Hmm, with those two patches on I get:

root@tigerlake:/usr/src/linux-2.6/tgl-build# ./test_progs -t fexit
#46 fentry_fexit:OK
#48 fexit_bpf2bpf:OK
#49 fexit_sleep:OK
#50 fexit_stress:OK
#51 fexit_test:OK
Summary: 5/9 PASSED, 0 SKIPPED, 0 FAILED

On the tigerlake, I suppose I'm doing something wrong on the other
machine because there it's even failing on the pre-ibt kernel image.

I'll go write up changelogs and stick these on.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Thu, Mar 10, 2022 at 09:37:31AM -0500, Steven Rostedt wrote:
> On Thu, 10 Mar 2022 14:47:18 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index acb50fb7ed2d..2d86d3c09d64 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -5354,6 +5381,11 @@ int modify_ftrace_direct(unsigned long ip,
> >  	mutex_lock(&direct_mutex);
> >  
> >  	mutex_lock(&ftrace_lock);
> > +
> > +	ip = ftrace_location(ip);
> > +	if (!ip)
> > +		goto out_unlock;
> > +
> 
> Perhaps this should go into find_direct_entry() instead, as I think you are
> adding it before all the find_direct_entry() callers.

Something like so then?

Index: linux-2.6/kernel/trace/ftrace.c
===================================================================
--- linux-2.6.orig/kernel/trace/ftrace.c
+++ linux-2.6/kernel/trace/ftrace.c
@@ -1575,7 +1575,7 @@ unsigned long ftrace_location_range(unsi
  * If @ip matches sym+0, return sym's ftrace location.
  * Otherwise, return 0.
  */
-unsigned long ftrace_location(unsigned long ip)
+unsigned long __ftrace_location(unsigned long ip, struct dyn_ftrace **recp)
 {
 	struct dyn_ftrace *rec;
 	unsigned long offset;
@@ -1591,13 +1591,22 @@ unsigned long ftrace_location(unsigned l
 			rec = lookup_rec(ip, ip + size - 1);
 	}
 
-	if (rec)
+	if (rec) {
+		if (recp)
+			*recp = rec;
+
 		return rec->ip;
+	}
 
 out:
 	return 0;
 }
 
+unsigned long ftrace_location(unsigned long ip)
+{
+	return __ftrace_location(ip, NULL);
+}
+
 /**
  * ftrace_text_reserved - return true if range contains an ftrace location
  * @start: start of range to search
@@ -2392,6 +2401,30 @@ static struct ftrace_hash *direct_functi
 static DEFINE_MUTEX(direct_mutex);
 int ftrace_direct_func_count;
 
+static struct ftrace_func_entry *
+find_direct_entry(unsigned long *ip, struct dyn_ftrace **recp, bool warn)
+{
+	struct ftrace_func_entry *entry;
+	struct dyn_ftrace *rec = NULL;
+
+	*ip = __ftrace_location(*ip, &rec);
+	if (!*ip)
+		return NULL;
+
+	if (recp)
+		*recp = rec;
+
+	entry = __ftrace_lookup_ip(direct_functions, *ip);
+	if (!entry) {
+		WARN_ON(rec->flags & FTRACE_FL_DIRECT);
+		return NULL;
+	}
+
+	WARN_ON(warn && !(rec->flags & FTRACE_FL_DIRECT));
+
+	return entry;
+}
+
 /*
  * Search the direct_functions hash to see if the given instruction pointer
  * has a direct caller attached to it.
@@ -2400,7 +2433,7 @@ unsigned long ftrace_find_rec_direct(uns
 {
 	struct ftrace_func_entry *entry;
 
-	entry = __ftrace_lookup_ip(direct_functions, ip);
+	entry = find_direct_entry(&ip, NULL, false);
 	if (!entry)
 		return 0;
 
@@ -5127,40 +5160,19 @@ int register_ftrace_direct(unsigned long
 	struct ftrace_direct_func *direct;
 	struct ftrace_func_entry *entry;
 	struct ftrace_hash *free_hash = NULL;
-	struct dyn_ftrace *rec;
+	struct dyn_ftrace *rec = NULL;
 	int ret = -ENODEV;
 
 	mutex_lock(&direct_mutex);
 
-	ip = ftrace_location(ip);
-	if (!ip)
+	entry = find_direct_entry(&ip, &rec, true);
+	if (!ip || !rec)
 		goto out_unlock;
 
-	/* See if there's a direct function at @ip already */
 	ret = -EBUSY;
-	if (ftrace_find_rec_direct(ip))
-		goto out_unlock;
-
-	ret = -ENODEV;
-	rec = lookup_rec(ip, ip);
-	if (!rec)
+	if (entry && entry->direct)
 		goto out_unlock;
 
-	/*
-	 * Check if the rec says it has a direct call but we didn't
-	 * find one earlier?
-	 */
-	if (WARN_ON(rec->flags & FTRACE_FL_DIRECT))
-		goto out_unlock;
-
-	/* Make sure the ip points to the exact record */
-	if (ip != rec->ip) {
-		ip = rec->ip;
-		/* Need to check this ip for a direct. */
-		if (ftrace_find_rec_direct(ip))
-			goto out_unlock;
-	}
-
 	ret = -ENOMEM;
 	direct = ftrace_find_direct_func(addr);
 	if (!direct) {
@@ -5209,33 +5221,6 @@ int register_ftrace_direct(unsigned long
 }
 EXPORT_SYMBOL_GPL(register_ftrace_direct);
 
-static struct ftrace_func_entry *find_direct_entry(unsigned long *ip,
-						   struct dyn_ftrace **recp)
-{
-	struct ftrace_func_entry *entry;
-	struct dyn_ftrace *rec;
-
-	rec = lookup_rec(*ip, *ip);
-	if (!rec)
-		return NULL;
-
-	entry = __ftrace_lookup_ip(direct_functions, rec->ip);
-	if (!entry) {
-		WARN_ON(rec->flags & FTRACE_FL_DIRECT);
-		return NULL;
-	}
-
-	WARN_ON(!(rec->flags & FTRACE_FL_DIRECT));
-
-	/* Passed in ip just needs to be on the call site */
-	*ip = rec->ip;
-
-	if (recp)
-		*recp = rec;
-
-	return entry;
-}
-
 int unregister_ftrace_direct(unsigned long ip, unsigned long addr)
 {
 	struct ftrace_direct_func *direct;
@@ -5245,11 +5230,7 @@ int unregister_ftrace_direct(unsigned lo
 
 	mutex_lock(&direct_mutex);
 
-	ip = ftrace_location(ip);
-	if (!ip)
-		goto out_unlock;
-
-	entry = find_direct_entry(&ip, NULL);
+	entry = find_direct_entry(&ip, NULL, true);
 	if (!entry)
 		goto out_unlock;
 
@@ -5382,11 +5363,7 @@ int modify_ftrace_direct(unsigned long i
 
 	mutex_lock(&ftrace_lock);
 
-	ip = ftrace_location(ip);
-	if (!ip)
-		goto out_unlock;
-
-	entry = find_direct_entry(&ip, &rec);
+	entry = find_direct_entry(&ip, &rec, true);
 	if (!entry)
 		goto out_unlock;
 

Re: [PATCH v4 00/45] x86: Kernel IBT

[Alexei Starovoitov]

On Fri, Mar 11, 2022 at 2:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Mar 10, 2022 at 05:29:11PM +0100, Peter Zijlstra wrote:
>
> > This seems to cure most of the rest. I'm still seeing one failure:
> >
> > libbpf: prog 'connect_v4_prog': BPF program load failed: Invalid argument
> > libbpf: failed to load program 'connect_v4_prog'
> > libbpf: failed to load object './connect4_prog.o'
> > test_fexit_bpf2bpf_common:FAIL:tgt_prog_load unexpected error: -22 (errno 22)
> > #48/4 fexit_bpf2bpf/func_replace_verify:FAIL
>
>
> Hmm, with those two patches on I get:
>
> root@tigerlake:/usr/src/linux-2.6/tgl-build# ./test_progs -t fexit
> #46 fentry_fexit:OK
> #48 fexit_bpf2bpf:OK
> #49 fexit_sleep:OK
> #50 fexit_stress:OK
> #51 fexit_test:OK
> Summary: 5/9 PASSED, 0 SKIPPED, 0 FAILED
>
> On the tigerlake, I suppose I'm doing something wrong on the other
> machine because there it's even failing on the pre-ibt kernel image.
>
> I'll go write up changelogs and stick these on.

What is the latest branch I can use to test it?

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Fri, Mar 11, 2022 at 09:09:38AM -0800, Alexei Starovoitov wrote:
> On Fri, Mar 11, 2022 at 2:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Thu, Mar 10, 2022 at 05:29:11PM +0100, Peter Zijlstra wrote:
> >
> > > This seems to cure most of the rest. I'm still seeing one failure:
> > >
> > > libbpf: prog 'connect_v4_prog': BPF program load failed: Invalid argument
> > > libbpf: failed to load program 'connect_v4_prog'
> > > libbpf: failed to load object './connect4_prog.o'
> > > test_fexit_bpf2bpf_common:FAIL:tgt_prog_load unexpected error: -22 (errno 22)
> > > #48/4 fexit_bpf2bpf/func_replace_verify:FAIL
> >
> >
> > Hmm, with those two patches on I get:
> >
> > root@tigerlake:/usr/src/linux-2.6/tgl-build# ./test_progs -t fexit
> > #46 fentry_fexit:OK
> > #48 fexit_bpf2bpf:OK
> > #49 fexit_sleep:OK
> > #50 fexit_stress:OK
> > #51 fexit_test:OK
> > Summary: 5/9 PASSED, 0 SKIPPED, 0 FAILED
> >
> > On the tigerlake, I suppose I'm doing something wrong on the other
> > machine because there it's even failing on the pre-ibt kernel image.
> >
> > I'll go write up changelogs and stick these on.
> 
> What is the latest branch I can use to test it?

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt

that also include bpf-next. Thanks!

Re: [PATCH v4 00/45] x86: Kernel IBT

[Alexei Starovoitov]

On Sat, Mar 12, 2022 at 7:44 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Mar 11, 2022 at 09:09:38AM -0800, Alexei Starovoitov wrote:
> > On Fri, Mar 11, 2022 at 2:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Thu, Mar 10, 2022 at 05:29:11PM +0100, Peter Zijlstra wrote:
> > >
> > > > This seems to cure most of the rest. I'm still seeing one failure:
> > > >
> > > > libbpf: prog 'connect_v4_prog': BPF program load failed: Invalid argument
> > > > libbpf: failed to load program 'connect_v4_prog'
> > > > libbpf: failed to load object './connect4_prog.o'
> > > > test_fexit_bpf2bpf_common:FAIL:tgt_prog_load unexpected error: -22 (errno 22)
> > > > #48/4 fexit_bpf2bpf/func_replace_verify:FAIL
> > >
> > >
> > > Hmm, with those two patches on I get:
> > >
> > > root@tigerlake:/usr/src/linux-2.6/tgl-build# ./test_progs -t fexit
> > > #46 fentry_fexit:OK
> > > #48 fexit_bpf2bpf:OK
> > > #49 fexit_sleep:OK
> > > #50 fexit_stress:OK
> > > #51 fexit_test:OK
> > > Summary: 5/9 PASSED, 0 SKIPPED, 0 FAILED
> > >
> > > On the tigerlake, I suppose I'm doing something wrong on the other
> > > machine because there it's even failing on the pre-ibt kernel image.
> > >
> > > I'll go write up changelogs and stick these on.
> >
> > What is the latest branch I can use to test it?
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt
>
> that also include bpf-next. Thanks!

Looks better.
During the build with gcc 8.5 I see:

arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
.ibt_endbr_seal, skipping
arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
.orc_unwind section, skipping
  LD [M]  crypto/async_tx/async_xor.ko
  LD [M]  crypto/authenc.ko
make[3]: *** [../scripts/Makefile.modfinal:61:
arch/x86/crypto/crc32c-intel.ko] Error 255
make[3]: *** Waiting for unfinished jobs....

but make clean cures it.
I suspect it's some missing makefile dependency.

and:
vmlinux.o: warning: objtool: ksys_unshare()+0x626: unreachable instruction
which stays even after make clean.

The rcu "false positive" is still there that causes
sporadic hangs during the boot.

The test_progs shows:
Summary: 228/1122 PASSED, 4 SKIPPED, 6 FAILED
(when I remove one test)

That test is actually crashing the kernel:
./test_progs -t mod_race
[   39.202593] bpf_testmod: loading out-of-tree module taints kernel.
[   39.303142] general protection fault, probably for non-canonical
address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN PTI
[   39.304610] KASAN: null-ptr-deref in range
[0x0000000000000000-0x0000000000000007]
[   39.305514] CPU: 9 PID: 1599 Comm: test_progs Tainted: G
O      5.17.0-rc7-02525-g5dd5efb53cf1 #4
[   39.306675] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
[   39.308036] RIP: 0010:do_init_module+0x9/0x6f0
[   39.308583] Code: fe ff ff e8 59 13 46 00 e9 7f fe ff ff e8 4f 13
46 00 e9 49 fe ff ff 66 2e 0f 1f 84 00 00 00 00 00 e8 cb 8d eb 1e 48
b8 00 00 <00> 00 00 fc ff df 41 57 49 89 ff 48 c7 c7 20 f6 5c 84 48 89
fa 41
[   39.310815] RSP: 0018:ffff88810f7e7aa0 EFLAGS: 00010282
[   39.311450] RAX: dffffc0000000000 RBX: 0000000000000009 RCX: ffffffff81283b16
[   39.312253] RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffffffffa0224c00
[   39.313031] RBP: ffff88810f7e7ac8 R08: 0000000000000000 R09: fffffbfff0b4d557
[   39.313813] R10: ffffffff85a6aab7 R11: fffffbfff0b4d556 R12: ffff88811171f518
[   39.314591] R13: dffffc0000000000 R14: ffffffffa0224c00 R15: ffff88810f7e7e50
[   39.315374] FS:  00007f8e1b981700(0000) GS:ffff8881f6a80000(0000)
knlGS:0000000000000000
[   39.316293] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   39.316984] CR2: 00007fdf39350ff0 CR3: 000000011952e006 CR4: 00000000003706e0
[   39.317860] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   39.318680] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   39.319467] Call Trace:
[   39.319744]  <TASK>
[   39.319982]  bpf_trampoline_6442471603_0+0x32/0x1000
[   39.320537]  do_init_module+0x5/0x6f0
[   39.320945]  load_module+0x77c0/0x9c00
[   39.321376]  ? module_frob_arch_sections+0x20/0x20
[   39.321892]  ? ima_post_read_file+0x161/0x180
[   39.322392]  ? ima_read_file+0x140/0x140
[   39.322827]  ? security_kernel_post_read_file+0x55/0xb0
[   39.323406]  ? __x64_sys_fsconfig+0x630/0x630
[   39.323889]  ? fput_many+0x1e/0x120
[   39.324285]  ? __do_sys_finit_module+0xf3/0x150
[   39.324822]  __do_sys_finit_module+0xf3/0x150
[   39.325311]  ? __ia32_sys_init_module+0xb0/0xb0
[   39.325826]  ? rcu_read_lock_held_common+0xe/0xa0
[   39.326349]  ? rcu_read_lock_sched_held+0x5a/0xc0
[   39.326869]  ? rcu_read_lock_bh_held+0xa0/0xa0
[   39.327362]  ? file_open_root+0x1f0/0x1f0
[   39.327812]  ? syscall_trace_enter.isra.17+0x184/0x250
[   39.328411]  do_syscall_64+0x38/0x80
[   39.328812]  entry_SYSCALL_64_after_hwframe+0x44/0xae

The test was designed to check whether the kernel bug is fixed.
If not it would crash the kernel.

Kumar,
you've added that test.
Could you please take a look at why it is crashing in Peter's tree?

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Sat, Mar 12, 2022 at 05:33:39PM -0800, Alexei Starovoitov wrote:
> During the build with gcc 8.5 I see:
> 
> arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> .ibt_endbr_seal, skipping
> arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> .orc_unwind section, skipping
>   LD [M]  crypto/async_tx/async_xor.ko
>   LD [M]  crypto/authenc.ko
> make[3]: *** [../scripts/Makefile.modfinal:61:
> arch/x86/crypto/crc32c-intel.ko] Error 255
> make[3]: *** Waiting for unfinished jobs....
> 
> but make clean cures it.
> I suspect it's some missing makefile dependency.

Yes, I recently ran into it; I've been trying to kick Makefile into
submission but have not had success yet. Will try again on Monday.

Problem appears to be that it will re-link .ko even though .o hasn't
changed, resulting in duplicate objtool runs. I've been trying to have
makefile generate .o.objtool empty file to serve as dependency marker to
avoid doing second objtool run, but like said, no luck yet.

> and:
> vmlinux.o: warning: objtool: ksys_unshare()+0x626: unreachable instruction
> which stays even after make clean.

Humm, I shall have to dig out gcc-8.5 then.

> The rcu "false positive" is still there that causes
> sporadic hangs during the boot.

I've merged fix for that yesterday, shall respin this ibt tree to
include that.

> The test_progs shows:
> Summary: 228/1122 PASSED, 4 SKIPPED, 6 FAILED
> (when I remove one test)
> 
> That test is actually crashing the kernel:
> ./test_progs -t mod_race

Argh, I wasn't seeing crashing, I'll prod with sharp stick.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Sun, Mar 13, 2022 at 09:52:14AM +0100, Peter Zijlstra wrote:
> On Sat, Mar 12, 2022 at 05:33:39PM -0800, Alexei Starovoitov wrote:
> > During the build with gcc 8.5 I see:
> > 
> > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > .ibt_endbr_seal, skipping
> > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > .orc_unwind section, skipping
> >   LD [M]  crypto/async_tx/async_xor.ko
> >   LD [M]  crypto/authenc.ko
> > make[3]: *** [../scripts/Makefile.modfinal:61:
> > arch/x86/crypto/crc32c-intel.ko] Error 255
> > make[3]: *** Waiting for unfinished jobs....
> > 
> > but make clean cures it.
> > I suspect it's some missing makefile dependency.
> 
> Yes, I recently ran into it; I've been trying to kick Makefile into
> submission but have not had success yet. Will try again on Monday.
> 
> Problem appears to be that it will re-link .ko even though .o hasn't
> changed, resulting in duplicate objtool runs. I've been trying to have
> makefile generate .o.objtool empty file to serve as dependency marker to
> avoid doing second objtool run, but like said, no luck yet.

Masahiro-san, I'm trying the below, but afaict it's not working because
the rule for the .o file itself:

$(multi-obj-m): FORCE
	$(call if_changed,link_multi-m)

will in fact update the timestamp of the .o file, even if if_changed
nops out the cmd. Concequently all rules that try to use if_changed with
this .o file as a dependency will find it newer and run anyway.


remake -x output of a fs/f2fs/ module (re)build:

     Prerequisite 'FORCE' of target 'fs/f2fs/f2fs.o' does not exist.
    Must remake target 'fs/f2fs/f2fs.o'.
../scripts/Makefile.build:454: target 'fs/f2fs/f2fs.o' does not exist
##>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
:
##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
    Successfully remade target file 'fs/f2fs/f2fs.o'.
   Prerequisite 'fs/f2fs/f2fs.o' is newer than target 'fs/f2fs/f2fs.mod'.
   Prerequisite 'FORCE' of target 'fs/f2fs/f2fs.mod' does not exist.
  Must remake target 'fs/f2fs/f2fs.mod'.
../scripts/Makefile.build:281: update target 'fs/f2fs/f2fs.mod' due to: fs/f2fs/f2fs.o
##>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
set -e;   { echo  fs/f2fs/dir.o fs/f2fs/file.o fs/f2fs/inode.o fs/f2fs/namei.o fs/f2fs/hash.o fs/f2fs/super.o fs/f2fs/inline.o fs/f2fs/checkpoint.o fs/f2fs/gc.o fs/f2fs/data.o fs/f2fs/node.o fs/f2fs/segment.o fs/f2fs/recovery.o fs/f2fs/shrinker.o fs/f2fs/extent_cache.o fs/f2fs/sysfs.o fs/f2fs/debug.o fs/f2fs/xattr.o fs/f2fs/acl.o fs/f2fs/iostat.o;  echo; } > fs/f2fs/f2fs.mod; printf '%s\n' 'cmd_fs/f2fs/f2fs.mod := { echo  fs/f2fs/dir.o fs/f2fs/file.o fs/f2fs/inode.o fs/f2fs/namei.o fs/f2fs/hash.o fs/f2fs/super.o fs/f2fs/inline.o fs/f2fs/checkpoint.o fs/f2fs/gc.o fs/f2fs/data.o fs/f2fs/node.o fs/f2fs/segment.o fs/f2fs/recovery.o fs/f2fs/shrinker.o fs/f2fs/extent_cache.o fs/f2fs/sysfs.o fs/f2fs/debug.o fs/f2fs/xattr.o fs/f2fs/acl.o fs/f2fs/iostat.o;  echo; } > fs/f2fs/f2fs.mod' > fs/f2fs/.f2fs.mod.cmd
##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
  Successfully remade target file 'fs/f2fs/f2fs.mod'.
   Prerequisite 'fs/f2fs/f2fs.o' is newer than target 'fs/f2fs/f2fs.objtool'.
   Prerequisite 'FORCE' of target 'fs/f2fs/f2fs.objtool' does not exist.
  Must remake target 'fs/f2fs/f2fs.objtool'.
../scripts/Makefile.build:287: update target 'fs/f2fs/f2fs.objtool' due to: fs/f2fs/f2fs.o
##>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
set -e;   { echo fs/f2fs/f2fs.o  ; ./tools/objtool/objtool orc generate  --module  --lto --ibt  --no-fp    --uaccess   fs/f2fs/f2fs.o ; } > fs/f2fs/f2fs.objtool; printf '%s\n' 'cmd_fs/f2fs/f2fs.objtool := { echo fs/f2fs/f2fs.o  ; ./tools/objtool/objtool orc generate  --module  --lto --ibt  --no-fp    --uaccess   fs/f2fs/f2fs.o ; } > fs/f2fs/f2fs.objtool' > fs/f2fs/.f2fs.objtool.cmd
##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
fs/f2fs/f2fs.o: warning: objtool: file already has .static_call_sites section, skipping
fs/f2fs/f2fs.o: warning: objtool: file already has .ibt_endbr_seal, skipping
fs/f2fs/f2fs.o: warning: objtool: file already has .orc_unwind section, skipping
../scripts/Makefile.build:286: *** [fs/f2fs/f2fs.objtool] error 255


Where we can see that we don't re-generate f2fs.o (empty command), but
then we do re-generate f2fs.mod because f2fs.o is newer and the same
happens for the new f2fs.objtool.

Help?

---
Index: linux-2.6/scripts/Makefile.build
===================================================================
--- linux-2.6.orig/scripts/Makefile.build
+++ linux-2.6/scripts/Makefile.build
@@ -92,6 +92,10 @@ ifdef CONFIG_LTO_CLANG
 targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
 endif
 
+ifdef CONFIG_X86_KERNEL_IBT
+targets-for-modules += $(patsubst %.o, %.objtool, $(filter %.o, $(obj-m)))
+endif
+
 ifdef need-modorder
 targets-for-modules += $(obj)/modules.order
 endif
@@ -276,6 +280,12 @@ cmd_mod = { \
 $(obj)/%.mod: $(obj)/%$(mod-prelink-ext).o FORCE
 	$(call if_changed,mod)
 
+cmd_objtool_mod =							\
+	{ echo $< $(cmd_objtool) ; } > $@
+
+$(obj)/%.objtool: $(obj)/%$(mod-prelink-ext).o FORCE
+	$(call if_changed,objtool_mod)
+
 quiet_cmd_cc_lst_c = MKLST   $@
       cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
 		     $(CONFIG_SHELL) $(srctree)/scripts/makelst $*.o \
Index: linux-2.6/scripts/Makefile.lib
===================================================================
--- linux-2.6.orig/scripts/Makefile.lib
+++ linux-2.6/scripts/Makefile.lib
@@ -552,9 +552,8 @@ objtool_args =								\
 	$(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount)		\
 	$(if $(CONFIG_SLS), --sls)
 
-cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
-cmd_objtool_mod = $(if $(objtool-enabled), $(objtool) $(objtool_args) $(@:.ko=.o) ; )
-cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
+cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $(@:.objtool=.o))
+cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$(@:.objtool=.o): $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
 
 endif # CONFIG_STACK_VALIDATION
 
@@ -575,8 +574,8 @@ $(obj)/%.o: objtool-enabled :=
 
 # instead run objtool on the module as a whole, right before
 # the final link pass with the linker script.
-%.ko: objtool-enabled = y
-%.ko: part-of-module := y
+$(obj)/%.objtool: objtool-enabled = y
+$(obj)/%.objtool: part-of-module := y
 
 else
 
Index: linux-2.6/scripts/Makefile.modfinal
===================================================================
--- linux-2.6.orig/scripts/Makefile.modfinal
+++ linux-2.6/scripts/Makefile.modfinal
@@ -32,7 +32,6 @@ ARCH_POSTLINK := $(wildcard $(srctree)/a
 
 quiet_cmd_ld_ko_o = LD [M]  $@
       cmd_ld_ko_o +=							\
-	$(cmd_objtool_mod)						\
 	$(LD) -r $(KBUILD_LDFLAGS)					\
 		$(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)		\
 		-T scripts/module.lds -o $@ $(filter %.o, $^);		\

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Sun, Mar 13, 2022 at 09:52:14AM +0100, Peter Zijlstra wrote:

> > and:
> > vmlinux.o: warning: objtool: ksys_unshare()+0x626: unreachable instruction
> > which stays even after make clean.
> 
> Humm, I shall have to dig out gcc-8.5 then.

Ha!, I could reproduce using the bpf-selftest .config for x86_64. Fixing
that (the __noreturn on __invalid_creds) immediately yields another one
on that .config in asm_exc_double_fault. And a missing ENDBR if you
don't build 32bit compat.

I'll go clean up ...


--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -551,6 +551,14 @@ SYM_CODE_START(\asmsym)
 	movq	$-1, ORIG_RAX(%rsp)	/* no syscall to restart */
 	call	\cfunc
 
+	/*
+	 * For some configurations exc_double_fault() is a noreturn
+	 */
+1:
+	.pushsection .discard.reachable
+	.long	1b - .
+	.popsection
+
 	jmp	paranoid_exit
 
 _ASM_NOKPROBE(\asmsym)
@@ -1440,6 +1448,7 @@ SYM_CODE_END(asm_exc_nmi)
  */
 SYM_CODE_START(ignore_sysret)
 	UNWIND_HINT_EMPTY
+	ENDBR
 	mov	$-ENOSYS, %eax
 	sysretl
 SYM_CODE_END(ignore_sysret)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index fcbc6885cc09..9ed9232af934 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -176,7 +176,7 @@ extern int set_cred_ucounts(struct cred *);
  * check for validity of credentials
  */
 #ifdef CONFIG_DEBUG_CREDENTIALS
-extern void __invalid_creds(const struct cred *, const char *, unsigned);
+extern void __noreturn __invalid_creds(const struct cred *, const char *, unsigned);
 extern void __validate_process_creds(struct task_struct *,
 				     const char *, unsigned);
 
diff --git a/kernel/cred.c b/kernel/cred.c
index 933155c96922..e10c15f51c1f 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -870,7 +870,7 @@ static void dump_invalid_creds(const struct cred *cred, const char *label,
 /*
  * report use of invalid credentials
  */
-void __invalid_creds(const struct cred *cred, const char *file, unsigned line)
+void __noreturn __invalid_creds(const struct cred *cred, const char *file, unsigned line)
 {
 	printk(KERN_ERR "CRED: Invalid credentials\n");
 	printk(KERN_ERR "CRED: At %s:%u\n", file, line);

Re: [PATCH v4 00/45] x86: Kernel IBT

[Kumar Kartikeya Dwivedi]

On Sun, Mar 13, 2022 at 07:03:39AM IST, Alexei Starovoitov wrote:
> On Sat, Mar 12, 2022 at 7:44 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Fri, Mar 11, 2022 at 09:09:38AM -0800, Alexei Starovoitov wrote:
> > > On Fri, Mar 11, 2022 at 2:40 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > On Thu, Mar 10, 2022 at 05:29:11PM +0100, Peter Zijlstra wrote:
> > > >
> > > > > This seems to cure most of the rest. I'm still seeing one failure:
> > > > >
> > > > > libbpf: prog 'connect_v4_prog': BPF program load failed: Invalid argument
> > > > > libbpf: failed to load program 'connect_v4_prog'
> > > > > libbpf: failed to load object './connect4_prog.o'
> > > > > test_fexit_bpf2bpf_common:FAIL:tgt_prog_load unexpected error: -22 (errno 22)
> > > > > #48/4 fexit_bpf2bpf/func_replace_verify:FAIL
> > > >
> > > >
> > > > Hmm, with those two patches on I get:
> > > >
> > > > root@tigerlake:/usr/src/linux-2.6/tgl-build# ./test_progs -t fexit
> > > > #46 fentry_fexit:OK
> > > > #48 fexit_bpf2bpf:OK
> > > > #49 fexit_sleep:OK
> > > > #50 fexit_stress:OK
> > > > #51 fexit_test:OK
> > > > Summary: 5/9 PASSED, 0 SKIPPED, 0 FAILED
> > > >
> > > > On the tigerlake, I suppose I'm doing something wrong on the other
> > > > machine because there it's even failing on the pre-ibt kernel image.
> > > >
> > > > I'll go write up changelogs and stick these on.
> > >
> > > What is the latest branch I can use to test it?
> >
> >   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt
> >
> > that also include bpf-next. Thanks!
>
> Looks better.
> During the build with gcc 8.5 I see:
>
> arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> .ibt_endbr_seal, skipping
> arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> .orc_unwind section, skipping
>   LD [M]  crypto/async_tx/async_xor.ko
>   LD [M]  crypto/authenc.ko
> make[3]: *** [../scripts/Makefile.modfinal:61:
> arch/x86/crypto/crc32c-intel.ko] Error 255
> make[3]: *** Waiting for unfinished jobs....
>
> but make clean cures it.
> I suspect it's some missing makefile dependency.
>
> and:
> vmlinux.o: warning: objtool: ksys_unshare()+0x626: unreachable instruction
> which stays even after make clean.
>
> The rcu "false positive" is still there that causes
> sporadic hangs during the boot.
>
> The test_progs shows:
> Summary: 228/1122 PASSED, 4 SKIPPED, 6 FAILED
> (when I remove one test)
>
> That test is actually crashing the kernel:
> ./test_progs -t mod_race
> [   39.202593] bpf_testmod: loading out-of-tree module taints kernel.
> [   39.303142] general protection fault, probably for non-canonical
> address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN PTI
> [   39.304610] KASAN: null-ptr-deref in range
> [0x0000000000000000-0x0000000000000007]
> [   39.305514] CPU: 9 PID: 1599 Comm: test_progs Tainted: G
> O      5.17.0-rc7-02525-g5dd5efb53cf1 #4
> [   39.306675] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.12.0-59-gc9ba5276e321-prebuilt.qemu.org 04/01/2014
> [   39.308036] RIP: 0010:do_init_module+0x9/0x6f0
> [   39.308583] Code: fe ff ff e8 59 13 46 00 e9 7f fe ff ff e8 4f 13
> 46 00 e9 49 fe ff ff 66 2e 0f 1f 84 00 00 00 00 00 e8 cb 8d eb 1e 48
> b8 00 00 <00> 00 00 fc ff df 41 57 49 89 ff 48 c7 c7 20 f6 5c 84 48 89
> fa 41
> [   39.310815] RSP: 0018:ffff88810f7e7aa0 EFLAGS: 00010282
> [   39.311450] RAX: dffffc0000000000 RBX: 0000000000000009 RCX: ffffffff81283b16
> [   39.312253] RDX: 0000000000000000 RSI: 0000000000000008 RDI: ffffffffa0224c00
> [   39.313031] RBP: ffff88810f7e7ac8 R08: 0000000000000000 R09: fffffbfff0b4d557
> [   39.313813] R10: ffffffff85a6aab7 R11: fffffbfff0b4d556 R12: ffff88811171f518
> [   39.314591] R13: dffffc0000000000 R14: ffffffffa0224c00 R15: ffff88810f7e7e50
> [   39.315374] FS:  00007f8e1b981700(0000) GS:ffff8881f6a80000(0000)
> knlGS:0000000000000000
> [   39.316293] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   39.316984] CR2: 00007fdf39350ff0 CR3: 000000011952e006 CR4: 00000000003706e0
> [   39.317860] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   39.318680] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   39.319467] Call Trace:
> [   39.319744]  <TASK>
> [   39.319982]  bpf_trampoline_6442471603_0+0x32/0x1000
> [   39.320537]  do_init_module+0x5/0x6f0
> [   39.320945]  load_module+0x77c0/0x9c00
> [   39.321376]  ? module_frob_arch_sections+0x20/0x20
> [   39.321892]  ? ima_post_read_file+0x161/0x180
> [   39.322392]  ? ima_read_file+0x140/0x140
> [   39.322827]  ? security_kernel_post_read_file+0x55/0xb0
> [   39.323406]  ? __x64_sys_fsconfig+0x630/0x630
> [   39.323889]  ? fput_many+0x1e/0x120
> [   39.324285]  ? __do_sys_finit_module+0xf3/0x150
> [   39.324822]  __do_sys_finit_module+0xf3/0x150
> [   39.325311]  ? __ia32_sys_init_module+0xb0/0xb0
> [   39.325826]  ? rcu_read_lock_held_common+0xe/0xa0
> [   39.326349]  ? rcu_read_lock_sched_held+0x5a/0xc0
> [   39.326869]  ? rcu_read_lock_bh_held+0xa0/0xa0
> [   39.327362]  ? file_open_root+0x1f0/0x1f0
> [   39.327812]  ? syscall_trace_enter.isra.17+0x184/0x250
> [   39.328411]  do_syscall_64+0x38/0x80
> [   39.328812]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> The test was designed to check whether the kernel bug is fixed.
> If not it would crash the kernel.
>
> Kumar,
> you've added that test.
> Could you please take a look at why it is crashing in Peter's tree?

The crash does not seem to be resurfacing the bug, AFAICT.

[ Note: I have no experience with trampoline code or IBT so what follows might
	be incorrect. ]

In case of fexit and fmod_ret, we call original function (but skip
X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
(gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.

This means for do_init_module module, orig_call += X86_PATCH_SIZE +
ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
function, which explains why I was seeing crash in the middle of
'mov edx, 0x10' instruction.

The diff below fixes the problem for me, and allows the test to pass.

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index b98e1c95bcc4..760c9a3c075f 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i

        ip_off = stack_size;

-       if (flags & BPF_TRAMP_F_SKIP_FRAME)
+       if (flags & BPF_TRAMP_F_SKIP_FRAME) {
                /* skip patched call instruction and point orig_call to actual
                 * body of the kernel function.
                 */
-               orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
+               if (is_endbr(*(u32 *)orig_call))
+                       orig_call += ENDBR_INSN_SIZE;
+               orig_call += X86_PATCH_SIZE;
+       }

        prog = image;

--
Kartikeya

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Mon, Mar 14, 2022 at 03:59:05PM +0100, Peter Zijlstra wrote:
> On Sun, Mar 13, 2022 at 09:52:14AM +0100, Peter Zijlstra wrote:
> > On Sat, Mar 12, 2022 at 05:33:39PM -0800, Alexei Starovoitov wrote:
> > > During the build with gcc 8.5 I see:
> > > 
> > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > .ibt_endbr_seal, skipping
> > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > .orc_unwind section, skipping
> > >   LD [M]  crypto/async_tx/async_xor.ko
> > >   LD [M]  crypto/authenc.ko
> > > make[3]: *** [../scripts/Makefile.modfinal:61:
> > > arch/x86/crypto/crc32c-intel.ko] Error 255
> > > make[3]: *** Waiting for unfinished jobs....
> > > 
> > > but make clean cures it.
> > > I suspect it's some missing makefile dependency.
> > 
> > Yes, I recently ran into it; I've been trying to kick Makefile into
> > submission but have not had success yet. Will try again on Monday.
> > 
> > Problem appears to be that it will re-link .ko even though .o hasn't
> > changed, resulting in duplicate objtool runs. I've been trying to have
> > makefile generate .o.objtool empty file to serve as dependency marker to
> > avoid doing second objtool run, but like said, no luck yet.
> 
> Masahiro-san, I'm trying the below, but afaict it's not working because
> the rule for the .o file itself:
> 
Ha, sleep, it is marvelous!

The below appears to be working as desired.

---
Index: linux-2.6/scripts/Makefile.build
===================================================================
--- linux-2.6.orig/scripts/Makefile.build
+++ linux-2.6/scripts/Makefile.build
@@ -86,12 +86,18 @@ ifdef need-builtin
 targets-for-builtin += $(obj)/built-in.a
 endif
 
-targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
+targets-for-modules :=
 
 ifdef CONFIG_LTO_CLANG
 targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
 endif
 
+ifdef CONFIG_X86_KERNEL_IBT
+targets-for-modules += $(patsubst %.o, %.objtool, $(filter %.o, $(obj-m)))
+endif
+
+targets-for-modules += $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
+
 ifdef need-modorder
 targets-for-modules += $(obj)/modules.order
 endif
@@ -276,6 +282,19 @@ cmd_mod = { \
 $(obj)/%.mod: $(obj)/%$(mod-prelink-ext).o FORCE
 	$(call if_changed,mod)
 
+#
+# Since objtool will re-write the file it will change the timestamps, therefore
+# it is critical that the %.objtool file gets a timestamp *after* objtool runs.
+#
+# Additionally, care must be had with ordering this rule against the other rules
+# that take %.o as a dependency.
+#
+cmd_objtool_mod =							\
+	true $(cmd_objtool) ; touch $@
+
+$(obj)/%.objtool: $(obj)/%$(mod-prelink-ext).o FORCE
+	$(call if_changed,objtool_mod)
+
 quiet_cmd_cc_lst_c = MKLST   $@
       cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
 		     $(CONFIG_SHELL) $(srctree)/scripts/makelst $*.o \
Index: linux-2.6/scripts/Makefile.lib
===================================================================
--- linux-2.6.orig/scripts/Makefile.lib
+++ linux-2.6/scripts/Makefile.lib
@@ -552,9 +552,8 @@ objtool_args =								\
 	$(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount)		\
 	$(if $(CONFIG_SLS), --sls)
 
-cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
-cmd_objtool_mod = $(if $(objtool-enabled), $(objtool) $(objtool_args) $(@:.ko=.o) ; )
-cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
+cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $(@:.objtool=.o))
+cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$(@:.objtool=.o): $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
 
 endif # CONFIG_STACK_VALIDATION
 
@@ -575,8 +574,8 @@ $(obj)/%.o: objtool-enabled :=
 
 # instead run objtool on the module as a whole, right before
 # the final link pass with the linker script.
-%.ko: objtool-enabled = y
-%.ko: part-of-module := y
+$(obj)/%.objtool: objtool-enabled = y
+$(obj)/%.objtool: part-of-module := y
 
 else
 
Index: linux-2.6/scripts/Makefile.modfinal
===================================================================
--- linux-2.6.orig/scripts/Makefile.modfinal
+++ linux-2.6/scripts/Makefile.modfinal
@@ -32,7 +32,6 @@ ARCH_POSTLINK := $(wildcard $(srctree)/a
 
 quiet_cmd_ld_ko_o = LD [M]  $@
       cmd_ld_ko_o +=							\
-	$(cmd_objtool_mod)						\
 	$(LD) -r $(KBUILD_LDFLAGS)					\
 		$(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)		\
 		-T scripts/module.lds -o $@ $(filter %.o, $^);		\

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 15, 2022 at 02:14:02AM +0530, Kumar Kartikeya Dwivedi wrote:

> [ Note: I have no experience with trampoline code or IBT so what follows might
> 	be incorrect. ]
> 
> In case of fexit and fmod_ret, we call original function (but skip
> X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
> cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
> (gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.
> 
> This means for do_init_module module, orig_call += X86_PATCH_SIZE +
> ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
> function, which explains why I was seeing crash in the middle of
> 'mov edx, 0x10' instruction.
> 
> The diff below fixes the problem for me, and allows the test to pass.
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index b98e1c95bcc4..760c9a3c075f 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> 
>         ip_off = stack_size;
> 
> -       if (flags & BPF_TRAMP_F_SKIP_FRAME)
> +       if (flags & BPF_TRAMP_F_SKIP_FRAME) {
>                 /* skip patched call instruction and point orig_call to actual
>                  * body of the kernel function.
>                  */
> -               orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
> +               if (is_endbr(*(u32 *)orig_call))
> +                       orig_call += ENDBR_INSN_SIZE;
> +               orig_call += X86_PATCH_SIZE;
> +       }
> 
>         prog = image;

Hmm, so I was under the impression that this was targeting the NOP from
emit_prologue(), and that has an unconditional ENDBR. If this is instead
targeting the 'start of random kernel function' then yes, what you
propose will work.

(obviously, once we go do more complicated CFI schemes, all this needs
revisiting yet again).

I don't seem able to run this mod_race test, it keeps saying:

  tgl-build# ./test_progs -v -t mod_race
  bpf_testmod.ko is already unloaded.
  Loading bpf_testmod.ko...
  Successfully loaded bpf_testmod.ko.
  Summary: 0/0 PASSED, 0 SKIPPED, 0 FAILED
  Successfully unloaded bpf_testmod.ko.

Which I'm taking to mean I'm doing it wrong... so I can't immediately
verify, but your proposal looks sane so I'll fold it in.

Thanks!

Re: [PATCH v4 00/45] x86: Kernel IBT

[Kumar Kartikeya Dwivedi]

On Tue, Mar 15, 2022 at 02:30:43PM IST, Peter Zijlstra wrote:
> On Tue, Mar 15, 2022 at 02:14:02AM +0530, Kumar Kartikeya Dwivedi wrote:
>
> > [ Note: I have no experience with trampoline code or IBT so what follows might
> > 	be incorrect. ]
> >
> > In case of fexit and fmod_ret, we call original function (but skip
> > X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
> > cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
> > (gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.
> >
> > This means for do_init_module module, orig_call += X86_PATCH_SIZE +
> > ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
> > function, which explains why I was seeing crash in the middle of
> > 'mov edx, 0x10' instruction.
> >
> > The diff below fixes the problem for me, and allows the test to pass.
> >
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index b98e1c95bcc4..760c9a3c075f 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> >
> >         ip_off = stack_size;
> >
> > -       if (flags & BPF_TRAMP_F_SKIP_FRAME)
> > +       if (flags & BPF_TRAMP_F_SKIP_FRAME) {
> >                 /* skip patched call instruction and point orig_call to actual
> >                  * body of the kernel function.
> >                  */
> > -               orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
> > +               if (is_endbr(*(u32 *)orig_call))
> > +                       orig_call += ENDBR_INSN_SIZE;
> > +               orig_call += X86_PATCH_SIZE;
> > +       }
> >
> >         prog = image;
>
> Hmm, so I was under the impression that this was targeting the NOP from
> emit_prologue(), and that has an unconditional ENDBR. If this is instead
> targeting the 'start of random kernel function' then yes, what you
> propose will work.
>
> (obviously, once we go do more complicated CFI schemes, all this needs
> revisiting yet again).
>
> I don't seem able to run this mod_race test, it keeps saying:
>
>   tgl-build# ./test_progs -v -t mod_race
>   bpf_testmod.ko is already unloaded.
>   Loading bpf_testmod.ko...
>   Successfully loaded bpf_testmod.ko.
>   Summary: 0/0 PASSED, 0 SKIPPED, 0 FAILED
>   Successfully unloaded bpf_testmod.ko.
>

`./test_progs -v -t bpf_mod_race` should work.

> Which I'm taking to mean I'm doing it wrong... so I can't immediately
> verify, but your proposal looks sane so I'll fold it in.
>
> Thanks!

--
Kartikeya

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 15, 2022 at 10:00:43AM +0100, Peter Zijlstra wrote:
> I don't seem able to run this mod_race test, it keeps saying:
> 
>   tgl-build# ./test_progs -v -t mod_race
>   bpf_testmod.ko is already unloaded.
>   Loading bpf_testmod.ko...
>   Successfully loaded bpf_testmod.ko.
>   Summary: 0/0 PASSED, 0 SKIPPED, 0 FAILED
>   Successfully unloaded bpf_testmod.ko.
> 
> Which I'm taking to mean I'm doing it wrong... 

That.. I was building the wrong tree, mod_race comes from bpf-next and I
was building a tree without that merged in... let me to see what it does
now.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 15, 2022 at 11:07:06AM +0100, Peter Zijlstra wrote:
> On Tue, Mar 15, 2022 at 10:00:43AM +0100, Peter Zijlstra wrote:
> > I don't seem able to run this mod_race test, it keeps saying:
> > 
> >   tgl-build# ./test_progs -v -t mod_race
> >   bpf_testmod.ko is already unloaded.
> >   Loading bpf_testmod.ko...
> >   Successfully loaded bpf_testmod.ko.
> >   Summary: 0/0 PASSED, 0 SKIPPED, 0 FAILED
> >   Successfully unloaded bpf_testmod.ko.
> > 
> > Which I'm taking to mean I'm doing it wrong... 
> 
> That.. I was building the wrong tree, mod_race comes from bpf-next and I
> was building a tree without that merged in... let me to see what it does
> now.

I can confirm, crashes without, fixed with. Thanks!

I've forced pushed a cleaned up tip/x86/core. Also:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/ibt.bpf

is a merge of that and bpf-next

Re: [PATCH v4 00/45] x86: Kernel IBT

[Masahiro Yamada]

On Mon, Mar 14, 2022 at 11:59 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Sun, Mar 13, 2022 at 09:52:14AM +0100, Peter Zijlstra wrote:
> > On Sat, Mar 12, 2022 at 05:33:39PM -0800, Alexei Starovoitov wrote:
> > > During the build with gcc 8.5 I see:
> > >
> > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > .ibt_endbr_seal, skipping
> > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > .orc_unwind section, skipping
> > >   LD [M]  crypto/async_tx/async_xor.ko
> > >   LD [M]  crypto/authenc.ko
> > > make[3]: *** [../scripts/Makefile.modfinal:61:
> > > arch/x86/crypto/crc32c-intel.ko] Error 255
> > > make[3]: *** Waiting for unfinished jobs....
> > >
> > > but make clean cures it.
> > > I suspect it's some missing makefile dependency.
> >
> > Yes, I recently ran into it; I've been trying to kick Makefile into
> > submission but have not had success yet. Will try again on Monday.
> >
> > Problem appears to be that it will re-link .ko even though .o hasn't
> > changed, resulting in duplicate objtool runs. I've been trying to have
> > makefile generate .o.objtool empty file to serve as dependency marker to
> > avoid doing second objtool run, but like said, no luck yet.
>
> Masahiro-san, I'm trying the below, but afaict it's not working because
> the rule for the .o file itself:
>
> $(multi-obj-m): FORCE
>         $(call if_changed,link_multi-m)
>
> will in fact update the timestamp of the .o file, even if if_changed
> nops out the cmd. Concequently all rules that try to use if_changed with
> this .o file as a dependency will find it newer and run anyway.
>
>
> remake -x output of a fs/f2fs/ module (re)build:
>
>      Prerequisite 'FORCE' of target 'fs/f2fs/f2fs.o' does not exist.
>     Must remake target 'fs/f2fs/f2fs.o'.
> ../scripts/Makefile.build:454: target 'fs/f2fs/f2fs.o' does not exist
> ##>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> :
> ##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>     Successfully remade target file 'fs/f2fs/f2fs.o'.
>    Prerequisite 'fs/f2fs/f2fs.o' is newer than target 'fs/f2fs/f2fs.mod'.
>    Prerequisite 'FORCE' of target 'fs/f2fs/f2fs.mod' does not exist.
>   Must remake target 'fs/f2fs/f2fs.mod'.
> ../scripts/Makefile.build:281: update target 'fs/f2fs/f2fs.mod' due to: fs/f2fs/f2fs.o
> ##>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> set -e;   { echo  fs/f2fs/dir.o fs/f2fs/file.o fs/f2fs/inode.o fs/f2fs/namei.o fs/f2fs/hash.o fs/f2fs/super.o fs/f2fs/inline.o fs/f2fs/checkpoint.o fs/f2fs/gc.o fs/f2fs/data.o fs/f2fs/node.o fs/f2fs/segment.o fs/f2fs/recovery.o fs/f2fs/shrinker.o fs/f2fs/extent_cache.o fs/f2fs/sysfs.o fs/f2fs/debug.o fs/f2fs/xattr.o fs/f2fs/acl.o fs/f2fs/iostat.o;  echo; } > fs/f2fs/f2fs.mod; printf '%s\n' 'cmd_fs/f2fs/f2fs.mod := { echo  fs/f2fs/dir.o fs/f2fs/file.o fs/f2fs/inode.o fs/f2fs/namei.o fs/f2fs/hash.o fs/f2fs/super.o fs/f2fs/inline.o fs/f2fs/checkpoint.o fs/f2fs/gc.o fs/f2fs/data.o fs/f2fs/node.o fs/f2fs/segment.o fs/f2fs/recovery.o fs/f2fs/shrinker.o fs/f2fs/extent_cache.o fs/f2fs/sysfs.o fs/f2fs/debug.o fs/f2fs/xattr.o fs/f2fs/acl.o fs/f2fs/iostat.o;  echo; } > fs/f2fs/f2fs.mod' > fs/f2fs/.f2fs.mod.cmd
> ##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>   Successfully remade target file 'fs/f2fs/f2fs.mod'.
>    Prerequisite 'fs/f2fs/f2fs.o' is newer than target 'fs/f2fs/f2fs.objtool'.
>    Prerequisite 'FORCE' of target 'fs/f2fs/f2fs.objtool' does not exist.
>   Must remake target 'fs/f2fs/f2fs.objtool'.
> ../scripts/Makefile.build:287: update target 'fs/f2fs/f2fs.objtool' due to: fs/f2fs/f2fs.o
> ##>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> set -e;   { echo fs/f2fs/f2fs.o  ; ./tools/objtool/objtool orc generate  --module  --lto --ibt  --no-fp    --uaccess   fs/f2fs/f2fs.o ; } > fs/f2fs/f2fs.objtool; printf '%s\n' 'cmd_fs/f2fs/f2fs.objtool := { echo fs/f2fs/f2fs.o  ; ./tools/objtool/objtool orc generate  --module  --lto --ibt  --no-fp    --uaccess   fs/f2fs/f2fs.o ; } > fs/f2fs/f2fs.objtool' > fs/f2fs/.f2fs.objtool.cmd
> ##<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> fs/f2fs/f2fs.o: warning: objtool: file already has .static_call_sites section, skipping
> fs/f2fs/f2fs.o: warning: objtool: file already has .ibt_endbr_seal, skipping
> fs/f2fs/f2fs.o: warning: objtool: file already has .orc_unwind section, skipping
> ../scripts/Makefile.build:286: *** [fs/f2fs/f2fs.objtool] error 255
>
>
> Where we can see that we don't re-generate f2fs.o (empty command), but
> then we do re-generate f2fs.mod because f2fs.o is newer and the same
> happens for the new f2fs.objtool.
>
> Help?


Help?

I had never noticed this thread before because
you did not CC me or kbuild ML.


Looking at the build system changes:
https://lore.kernel.org/all/20220308154319.528181453@infradead.org/
https://lore.kernel.org/all/20220308154319.822545231@infradead.org/

Both patches are wrong.
So is the fix-up you appended lator in this thread.

Apparently, you were screwing up Kbuild in a brown paper bag.
So scared.







>
> ---
> Index: linux-2.6/scripts/Makefile.build
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.build
> +++ linux-2.6/scripts/Makefile.build
> @@ -92,6 +92,10 @@ ifdef CONFIG_LTO_CLANG
>  targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
>  endif
>
> +ifdef CONFIG_X86_KERNEL_IBT
> +targets-for-modules += $(patsubst %.o, %.objtool, $(filter %.o, $(obj-m)))
> +endif
> +
>  ifdef need-modorder
>  targets-for-modules += $(obj)/modules.order
>  endif
> @@ -276,6 +280,12 @@ cmd_mod = { \
>  $(obj)/%.mod: $(obj)/%$(mod-prelink-ext).o FORCE
>         $(call if_changed,mod)
>
> +cmd_objtool_mod =                                                      \
> +       { echo $< $(cmd_objtool) ; } > $@
> +
> +$(obj)/%.objtool: $(obj)/%$(mod-prelink-ext).o FORCE
> +       $(call if_changed,objtool_mod)
> +
>  quiet_cmd_cc_lst_c = MKLST   $@
>        cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
>                      $(CONFIG_SHELL) $(srctree)/scripts/makelst $*.o \
> Index: linux-2.6/scripts/Makefile.lib
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.lib
> +++ linux-2.6/scripts/Makefile.lib
> @@ -552,9 +552,8 @@ objtool_args =                                                              \
>         $(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount)             \
>         $(if $(CONFIG_SLS), --sls)
>
> -cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
> -cmd_objtool_mod = $(if $(objtool-enabled), $(objtool) $(objtool_args) $(@:.ko=.o) ; )
> -cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
> +cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $(@:.objtool=.o))
> +cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$(@:.objtool=.o): $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
>
>  endif # CONFIG_STACK_VALIDATION
>
> @@ -575,8 +574,8 @@ $(obj)/%.o: objtool-enabled :=
>
>  # instead run objtool on the module as a whole, right before
>  # the final link pass with the linker script.
> -%.ko: objtool-enabled = y
> -%.ko: part-of-module := y
> +$(obj)/%.objtool: objtool-enabled = y
> +$(obj)/%.objtool: part-of-module := y
>
>  else
>
> Index: linux-2.6/scripts/Makefile.modfinal
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.modfinal
> +++ linux-2.6/scripts/Makefile.modfinal
> @@ -32,7 +32,6 @@ ARCH_POSTLINK := $(wildcard $(srctree)/a
>
>  quiet_cmd_ld_ko_o = LD [M]  $@
>        cmd_ld_ko_o +=                                                   \
> -       $(cmd_objtool_mod)                                              \
>         $(LD) -r $(KBUILD_LDFLAGS)                                      \
>                 $(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)              \
>                 -T scripts/module.lds -o $@ $(filter %.o, $^);          \



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 00/45] x86: Kernel IBT

[Masahiro Yamada]

On Tue, Mar 15, 2022 at 5:15 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Mar 14, 2022 at 03:59:05PM +0100, Peter Zijlstra wrote:
> > On Sun, Mar 13, 2022 at 09:52:14AM +0100, Peter Zijlstra wrote:
> > > On Sat, Mar 12, 2022 at 05:33:39PM -0800, Alexei Starovoitov wrote:
> > > > During the build with gcc 8.5 I see:
> > > >
> > > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > > .ibt_endbr_seal, skipping
> > > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > > .orc_unwind section, skipping
> > > >   LD [M]  crypto/async_tx/async_xor.ko
> > > >   LD [M]  crypto/authenc.ko
> > > > make[3]: *** [../scripts/Makefile.modfinal:61:
> > > > arch/x86/crypto/crc32c-intel.ko] Error 255
> > > > make[3]: *** Waiting for unfinished jobs....
> > > >
> > > > but make clean cures it.
> > > > I suspect it's some missing makefile dependency.
> > >
> > > Yes, I recently ran into it; I've been trying to kick Makefile into
> > > submission but have not had success yet. Will try again on Monday.
> > >
> > > Problem appears to be that it will re-link .ko even though .o hasn't
> > > changed, resulting in duplicate objtool runs. I've been trying to have
> > > makefile generate .o.objtool empty file to serve as dependency marker to
> > > avoid doing second objtool run, but like said, no luck yet.
> >
> > Masahiro-san, I'm trying the below, but afaict it's not working because
> > the rule for the .o file itself:
> >
> Ha, sleep, it is marvelous!
>
> The below appears to be working as desired.
>
> ---
> Index: linux-2.6/scripts/Makefile.build
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.build
> +++ linux-2.6/scripts/Makefile.build
> @@ -86,12 +86,18 @@ ifdef need-builtin
>  targets-for-builtin += $(obj)/built-in.a
>  endif
>
> -targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> +targets-for-modules :=


Why do you need to change this line?



>
>  ifdef CONFIG_LTO_CLANG
>  targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
>  endif
>
> +ifdef CONFIG_X86_KERNEL_IBT
> +targets-for-modules += $(patsubst %.o, %.objtool, $(filter %.o, $(obj-m)))
> +endif
> +
> +targets-for-modules += $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> +
>  ifdef need-modorder
>  targets-for-modules += $(obj)/modules.order
>  endif
> @@ -276,6 +282,19 @@ cmd_mod = { \
>  $(obj)/%.mod: $(obj)/%$(mod-prelink-ext).o FORCE
>         $(call if_changed,mod)
>
> +#
> +# Since objtool will re-write the file it will change the timestamps, therefore
> +# it is critical that the %.objtool file gets a timestamp *after* objtool runs.


Thanks for explaining how stupidly this works.
NACK.


I guess re-using the current clang-lto rule is much cleaner.
(but please rename  %.lto.o  to  %.prelink.o)


Roughly like this:


if CONFIG_LTO_CLANG || CONFIG_X86_KERNEL_IBT

$(obj)/%.prelink:  $(obj)/%.o FORCE
        [  $(LD) if CONFIG_LTO_CLANG ]   +  $(cmd_objtool)

endif







> +#
> +# Additionally, care must be had with ordering this rule against the other rules
> +# that take %.o as a dependency.
> +#
> +cmd_objtool_mod =                                                      \
> +       true $(cmd_objtool) ; touch $@
> +
> +$(obj)/%.objtool: $(obj)/%$(mod-prelink-ext).o FORCE
> +       $(call if_changed,objtool_mod)
> +
>  quiet_cmd_cc_lst_c = MKLST   $@
>        cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
>                      $(CONFIG_SHELL) $(srctree)/scripts/makelst $*.o \
> Index: linux-2.6/scripts/Makefile.lib
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.lib
> +++ linux-2.6/scripts/Makefile.lib
> @@ -552,9 +552,8 @@ objtool_args =                                                              \
>         $(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount)             \
>         $(if $(CONFIG_SLS), --sls)
>
> -cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
> -cmd_objtool_mod = $(if $(objtool-enabled), $(objtool) $(objtool_args) $(@:.ko=.o) ; )
> -cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
> +cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $(@:.objtool=.o))
> +cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$(@:.objtool=.o): $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
>
>  endif # CONFIG_STACK_VALIDATION
>
> @@ -575,8 +574,8 @@ $(obj)/%.o: objtool-enabled :=
>
>  # instead run objtool on the module as a whole, right before
>  # the final link pass with the linker script.
> -%.ko: objtool-enabled = y
> -%.ko: part-of-module := y
> +$(obj)/%.objtool: objtool-enabled = y
> +$(obj)/%.objtool: part-of-module := y
>
>  else
>
> Index: linux-2.6/scripts/Makefile.modfinal
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.modfinal
> +++ linux-2.6/scripts/Makefile.modfinal
> @@ -32,7 +32,6 @@ ARCH_POSTLINK := $(wildcard $(srctree)/a
>
>  quiet_cmd_ld_ko_o = LD [M]  $@
>        cmd_ld_ko_o +=                                                   \
> -       $(cmd_objtool_mod)                                              \
>         $(LD) -r $(KBUILD_LDFLAGS)                                      \
>                 $(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)              \
>                 -T scripts/module.lds -o $@ $(filter %.o, $^);          \



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4 00/45] x86: Kernel IBT

[Alexei Starovoitov]

On Mon, Mar 14, 2022 at 1:44 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
>
> The crash does not seem to be resurfacing the bug, AFAICT.
>
> [ Note: I have no experience with trampoline code or IBT so what follows might
>         be incorrect. ]
>
> In case of fexit and fmod_ret, we call original function (but skip
> X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
> cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
> (gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.
>
> This means for do_init_module module, orig_call += X86_PATCH_SIZE +
> ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
> function, which explains why I was seeing crash in the middle of
> 'mov edx, 0x10' instruction.
>
> The diff below fixes the problem for me, and allows the test to pass.
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index b98e1c95bcc4..760c9a3c075f 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
>
>         ip_off = stack_size;
>
> -       if (flags & BPF_TRAMP_F_SKIP_FRAME)
> +       if (flags & BPF_TRAMP_F_SKIP_FRAME) {
>                 /* skip patched call instruction and point orig_call to actual
>                  * body of the kernel function.
>                  */
> -               orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
> +               if (is_endbr(*(u32 *)orig_call))
> +                       orig_call += ENDBR_INSN_SIZE;
> +               orig_call += X86_PATCH_SIZE;
> +       }
>

Thanks Kumar!
The bpf trampoline can attach to both indirect and non-indirect
functions. My understanding is that only indirect targets will have
endbr first insn. So the fix totally makes sense.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 15, 2022 at 10:00:43AM +0100, Peter Zijlstra wrote:
> On Tue, Mar 15, 2022 at 02:14:02AM +0530, Kumar Kartikeya Dwivedi wrote:
> 
> > [ Note: I have no experience with trampoline code or IBT so what follows might
> > 	be incorrect. ]
> > 
> > In case of fexit and fmod_ret, we call original function (but skip
> > X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
> > cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
> > (gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.
> > 
> > This means for do_init_module module, orig_call += X86_PATCH_SIZE +
> > ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
> > function, which explains why I was seeing crash in the middle of
> > 'mov edx, 0x10' instruction.
> > 
> > The diff below fixes the problem for me, and allows the test to pass.
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > index b98e1c95bcc4..760c9a3c075f 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > 
> >         ip_off = stack_size;
> > 
> > -       if (flags & BPF_TRAMP_F_SKIP_FRAME)
> > +       if (flags & BPF_TRAMP_F_SKIP_FRAME) {
> >                 /* skip patched call instruction and point orig_call to actual
> >                  * body of the kernel function.
> >                  */
> > -               orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
> > +               if (is_endbr(*(u32 *)orig_call))
> > +                       orig_call += ENDBR_INSN_SIZE;
> > +               orig_call += X86_PATCH_SIZE;
> > +       }
> > 
> >         prog = image;
> 
> Hmm, so I was under the impression that this was targeting the NOP from
> emit_prologue(), and that has an unconditional ENDBR. If this is instead
> targeting the 'start of random kernel function' then yes, what you
> propose will work.

Can you confirm that orig_call can be any kernel function? Because if
so, I'm thinking it will still do the wrong thing for a notrace
function, that will not have a __fentry__ site, so unconditionally
skipping those 5 bytes will place you somewhere non-sensible.

This would not be a new issue; but perhaps it should be clarified and or
fixed.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Kumar Kartikeya Dwivedi]

On Wed, Mar 16, 2022 at 03:05:38PM IST, Peter Zijlstra wrote:
> On Tue, Mar 15, 2022 at 10:00:43AM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 15, 2022 at 02:14:02AM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > > [ Note: I have no experience with trampoline code or IBT so what follows might
> > > 	be incorrect. ]
> > >
> > > In case of fexit and fmod_ret, we call original function (but skip
> > > X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
> > > cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
> > > (gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.
> > >
> > > This means for do_init_module module, orig_call += X86_PATCH_SIZE +
> > > ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
> > > function, which explains why I was seeing crash in the middle of
> > > 'mov edx, 0x10' instruction.
> > >
> > > The diff below fixes the problem for me, and allows the test to pass.
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index b98e1c95bcc4..760c9a3c075f 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > >
> > >         ip_off = stack_size;
> > >
> > > -       if (flags & BPF_TRAMP_F_SKIP_FRAME)
> > > +       if (flags & BPF_TRAMP_F_SKIP_FRAME) {
> > >                 /* skip patched call instruction and point orig_call to actual
> > >                  * body of the kernel function.
> > >                  */
> > > -               orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
> > > +               if (is_endbr(*(u32 *)orig_call))
> > > +                       orig_call += ENDBR_INSN_SIZE;
> > > +               orig_call += X86_PATCH_SIZE;
> > > +       }
> > >
> > >         prog = image;
> >
> > Hmm, so I was under the impression that this was targeting the NOP from
> > emit_prologue(), and that has an unconditional ENDBR. If this is instead
> > targeting the 'start of random kernel function' then yes, what you
> > propose will work.
>
> Can you confirm that orig_call can be any kernel function? Because if
> so, I'm thinking it will still do the wrong thing for a notrace
> function, that will not have a __fentry__ site, so unconditionally
> skipping those 5 bytes will place you somewhere non-sensible.
>

It fails for notrace functions, e.g. considering fentry prog, when
bpf_trampoline_link_prog -> bpf_trampoline_update eventually calls
register_fentry -> bpf_arch_text_poke, old_addr is NULL, so nop_insn is copied
to old_insn, and then that memcmp(ip, old_insn, X86_PATCH_SIZE) should fail, so
it would return -EBUSY. If you consider modify_fentry case, then register_fentry
for earlier prog must have succeeded, so it must not be notrace function.

The orig_call adjustment is only done for fexit and fmod_ret (they set CALL_ORIG
and SKIP_FRAME flags), because in case of just fentry we just continue after
ret, instead of emitting call to original function in trampoline, for those too
the bpf_arch_text_poke should fail, for the same reason as above.

> This would not be a new issue; but perhaps it should be clarified and or
> fixed.

Based on my inspection it looks fine, others can correct me if I'm wrong.

--
Kartikeya

Re: [PATCH v4 00/45] x86: Kernel IBT

[Masahiro Yamada]

On Tue, Mar 15, 2022 at 5:15 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Mar 14, 2022 at 03:59:05PM +0100, Peter Zijlstra wrote:
> > On Sun, Mar 13, 2022 at 09:52:14AM +0100, Peter Zijlstra wrote:
> > > On Sat, Mar 12, 2022 at 05:33:39PM -0800, Alexei Starovoitov wrote:
> > > > During the build with gcc 8.5 I see:
> > > >
> > > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > > .ibt_endbr_seal, skipping
> > > > arch/x86/crypto/crc32c-intel.o: warning: objtool: file already has
> > > > .orc_unwind section, skipping
> > > >   LD [M]  crypto/async_tx/async_xor.ko
> > > >   LD [M]  crypto/authenc.ko
> > > > make[3]: *** [../scripts/Makefile.modfinal:61:
> > > > arch/x86/crypto/crc32c-intel.ko] Error 255
> > > > make[3]: *** Waiting for unfinished jobs....
> > > >
> > > > but make clean cures it.
> > > > I suspect it's some missing makefile dependency.
> > >
> > > Yes, I recently ran into it; I've been trying to kick Makefile into
> > > submission but have not had success yet. Will try again on Monday.
> > >
> > > Problem appears to be that it will re-link .ko even though .o hasn't
> > > changed, resulting in duplicate objtool runs. I've been trying to have
> > > makefile generate .o.objtool empty file to serve as dependency marker to
> > > avoid doing second objtool run, but like said, no luck yet.
> >
> > Masahiro-san, I'm trying the below, but afaict it's not working because
> > the rule for the .o file itself:
> >
> Ha, sleep, it is marvelous!
>
> The below appears to be working as desired.
>
> ---
> Index: linux-2.6/scripts/Makefile.build
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.build
> +++ linux-2.6/scripts/Makefile.build
> @@ -86,12 +86,18 @@ ifdef need-builtin
>  targets-for-builtin += $(obj)/built-in.a
>  endif
>
> -targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> +targets-for-modules :=
>
>  ifdef CONFIG_LTO_CLANG
>  targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
>  endif
>
> +ifdef CONFIG_X86_KERNEL_IBT
> +targets-for-modules += $(patsubst %.o, %.objtool, $(filter %.o, $(obj-m)))
> +endif
> +
> +targets-for-modules += $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> +
>  ifdef need-modorder
>  targets-for-modules += $(obj)/modules.order
>  endif
> @@ -276,6 +282,19 @@ cmd_mod = { \
>  $(obj)/%.mod: $(obj)/%$(mod-prelink-ext).o FORCE
>         $(call if_changed,mod)
>
> +#
> +# Since objtool will re-write the file it will change the timestamps, therefore
> +# it is critical that the %.objtool file gets a timestamp *after* objtool runs.
> +#
> +# Additionally, care must be had with ordering this rule against the other rules
> +# that take %.o as a dependency.
> +#
> +cmd_objtool_mod =                                                      \
> +       true $(cmd_objtool) ; touch $@
> +
> +$(obj)/%.objtool: $(obj)/%$(mod-prelink-ext).o FORCE
> +       $(call if_changed,objtool_mod)
> +
>  quiet_cmd_cc_lst_c = MKLST   $@
>        cmd_cc_lst_c = $(CC) $(c_flags) -g -c -o $*.o $< && \
>                      $(CONFIG_SHELL) $(srctree)/scripts/makelst $*.o \
> Index: linux-2.6/scripts/Makefile.lib
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.lib
> +++ linux-2.6/scripts/Makefile.lib
> @@ -552,9 +552,8 @@ objtool_args =                                                              \
>         $(if $(CONFIG_FTRACE_MCOUNT_USE_OBJTOOL), --mcount)             \
>         $(if $(CONFIG_SLS), --sls)
>
> -cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $@)
> -cmd_objtool_mod = $(if $(objtool-enabled), $(objtool) $(objtool_args) $(@:.ko=.o) ; )
> -cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
> +cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool_args) $(@:.objtool=.o))
> +cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$(@:.objtool=.o): $$(wildcard $(objtool))' ; } >> $(dot-target).cmd)
>
>  endif # CONFIG_STACK_VALIDATION
>
> @@ -575,8 +574,8 @@ $(obj)/%.o: objtool-enabled :=
>
>  # instead run objtool on the module as a whole, right before
>  # the final link pass with the linker script.
> -%.ko: objtool-enabled = y
> -%.ko: part-of-module := y
> +$(obj)/%.objtool: objtool-enabled = y
> +$(obj)/%.objtool: part-of-module := y
>
>  else
>
> Index: linux-2.6/scripts/Makefile.modfinal
> ===================================================================
> --- linux-2.6.orig/scripts/Makefile.modfinal
> +++ linux-2.6/scripts/Makefile.modfinal
> @@ -32,7 +32,6 @@ ARCH_POSTLINK := $(wildcard $(srctree)/a
>
>  quiet_cmd_ld_ko_o = LD [M]  $@
>        cmd_ld_ko_o +=                                                   \
> -       $(cmd_objtool_mod)                                              \
>         $(LD) -r $(KBUILD_LDFLAGS)                                      \
>                 $(KBUILD_LDFLAGS_MODULE) $(LDFLAGS_MODULE)              \
>                 -T scripts/module.lds -o $@ $(filter %.o, $^);          \





I wrote cleaner code for the Kbuild part.

This replaces
  scripts/Makefile*
  scripts/mod/modpost.c
of ("x86/alternative: Use .ibt_endbr_seal to seal indirect calls")

If there is more time, I have an even cleaner idea.



diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index a4b89b757287..12812cbb54cd 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -88,8 +88,8 @@ endif

 targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))

-ifdef CONFIG_LTO_CLANG
-targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
+ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
+targets-for-modules += $(patsubst %.o, %.prelink.o, $(filter %.o, $(obj-m)))
 endif

 ifdef need-modorder
@@ -170,7 +170,7 @@ ifdef CONFIG_MODVERSIONS
 #   the actual value of the checksum generated by genksyms
 # o remove .tmp_<file>.o to <file>.o

-ifdef CONFIG_LTO_CLANG
+ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
 # Generate .o.symversions files for each .o with exported symbols,
and link these
 # to the kernel and/or modules at the end.
 cmd_modversions_c =
         \
@@ -230,6 +230,7 @@ objtool := $(objtree)/tools/objtool/objtool
 objtool_args =                                                         \
        $(if $(CONFIG_UNWINDER_ORC),orc generate,check)                 \
        $(if $(part-of-module), --module)                               \
+       $(if $(CONFIG_X86_KERNEL_IBT), --lto --ibt)                     \
        $(if $(CONFIG_FRAME_POINTER),, --no-fp)                         \
        $(if $(CONFIG_GCOV_KERNEL)$(CONFIG_LTO_CLANG), --no-unreachable)\
        $(if $(CONFIG_RETPOLINE), --retpoline)                          \
@@ -242,7 +243,7 @@ cmd_gen_objtooldep = $(if $(objtool-enabled), {
echo ; echo '$@: $$(wildcard $(o

 endif # CONFIG_STACK_VALIDATION

-ifdef CONFIG_LTO_CLANG
+ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)

 # Skip objtool for LLVM bitcode
 $(obj)/%.o: objtool-enabled :=
@@ -288,24 +289,24 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE
        $(call if_changed_rule,cc_o_c)
        $(call cmd,force_checksrc)

-ifdef CONFIG_LTO_CLANG
+ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
 # Module .o files may contain LLVM bitcode, compile them into native code
 # before ELF processing
-quiet_cmd_cc_lto_link_modules = LTO [M] $@
-cmd_cc_lto_link_modules =                                              \
+quiet_cmd_cc_prelink_modules = LD [M]  $@
+      cmd_cc_prelink_modules =                                         \
        $(LD) $(ld_flags) -r -o $@                                      \
-               $(shell [ -s $(@:.lto.o=.o.symversions) ] &&            \
-                       echo -T $(@:.lto.o=.o.symversions))             \
+               $(shell [ -s $(@:.prelink.o=.o.symversions) ] &&
         \
+                       echo -T $(@:.prelink.o=.o.symversions))         \
                --whole-archive $(filter-out FORCE,$^)                  \
                $(cmd_objtool)

 # objtool was skipped for LLVM bitcode, run it now that we have compiled
 # modules into native code
-$(obj)/%.lto.o: objtool-enabled = y
-$(obj)/%.lto.o: part-of-module := y
+$(obj)/%.prelink.o: objtool-enabled = y
+$(obj)/%.prelink.o: part-of-module := y

-$(obj)/%.lto.o: $(obj)/%.o FORCE
-       $(call if_changed,cc_lto_link_modules)
+$(obj)/%.prelink.o: $(obj)/%.o FORCE
+       $(call if_changed,cc_prelink_modules)
 endif

 cmd_mod = { \
@@ -469,7 +470,7 @@ $(obj)/lib.a: $(lib-y) FORCE
 # Do not replace $(filter %.o,^) with $(real-prereqs). When a single object
 # module is turned into a multi object module, $^ will contain header file
 # dependencies recorded in the .*.cmd file.
-ifdef CONFIG_LTO_CLANG
+ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
 quiet_cmd_link_multi-m = AR [M]  $@
 cmd_link_multi-m =                                             \
        $(cmd_update_lto_symversions);                          \
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 79be57fdd32a..8bfc9238237c 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -230,11 +230,11 @@ dtc_cpp_flags  = -Wp,-MMD,$(depfile).pre.tmp
-nostdinc                    \
                 $(addprefix -I,$(DTC_INCLUDE))                          \
                 -undef -D__DTS__

-ifeq ($(CONFIG_LTO_CLANG),y)
+ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
 # With CONFIG_LTO_CLANG, .o files in modules might be LLVM bitcode, so we
 # need to run LTO to compile them into native code (.lto.o) before further
 # processing.
-mod-prelink-ext := .lto
+mod-prelink-ext := .prelink
 endif

 # Useful for describing the dependency of composite objects
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 6bfa33217914..09c3ab0a9b37 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1989,9 +1989,9 @@ static char *remove_dot(char *s)
                if (m && (s[n + m] == '.' || s[n + m] == 0))
                        s[n] = 0;

-               /* strip trailing .lto */
-               if (strends(s, ".lto"))
-                       s[strlen(s) - 4] = '\0';
+               /* strip trailing .prelink */
+               if (strends(s, ".prelink"))
+                       s[strlen(s) - 8] = '\0';
        }
        return s;
 }
@@ -2015,9 +2015,9 @@ static void read_symbols(const char *modname)
                /* strip trailing .o */
                tmp = NOFAIL(strdup(modname));
                tmp[strlen(tmp) - 2] = '\0';
-               /* strip trailing .lto */
-               if (strends(tmp, ".lto"))
-                       tmp[strlen(tmp) - 4] = '\0';
+               /* strip trailing .prelink */
+               if (strends(tmp, ".prelink"))
+                       tmp[strlen(tmp) - 8] = '\0';
                mod = new_module(tmp);
                free(tmp);

        }













--
Best Regards
Masahiro Yamada

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Wed, Mar 16, 2022 at 01:26:25AM +0900, Masahiro Yamada wrote:
> Help?
> 
> I had never noticed this thread before because
> you did not CC me or kbuild ML.

I thought I did.. the copy in my sent folder has you on Cc. Sorry if it
went MIA. I'll go look at the patch.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Wed, Mar 16, 2022 at 01:28:08AM +0900, Masahiro Yamada wrote:
> On Tue, Mar 15, 2022 at 5:15 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > Index: linux-2.6/scripts/Makefile.build
> > ===================================================================
> > --- linux-2.6.orig/scripts/Makefile.build
> > +++ linux-2.6/scripts/Makefile.build
> > @@ -86,12 +86,18 @@ ifdef need-builtin
> >  targets-for-builtin += $(obj)/built-in.a
> >  endif
> >
> > -targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> > +targets-for-modules :=
> 
> 
> Why do you need to change this line?
> 
> 
> 
> >
> >  ifdef CONFIG_LTO_CLANG
> >  targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
> >  endif
> >
> > +ifdef CONFIG_X86_KERNEL_IBT
> > +targets-for-modules += $(patsubst %.o, %.objtool, $(filter %.o, $(obj-m)))
> > +endif
> > +
> > +targets-for-modules += $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> > +
> >  ifdef need-modorder
> >  targets-for-modules += $(obj)/modules.order
> >  endif

The thinking was that by having the .objtool rule before the .mod rule,
objtool runs first. If mod runs before objtool, objtool will change the
timestamp and then mod will get remade, even if nothing's changed.

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Fri, Mar 18, 2022 at 03:15:59AM +0900, Masahiro Yamada wrote:


This is somewhat similar to my first attempt, except I thought it had a
extra/superflous link pass in it..

> @@ -288,24 +289,24 @@ $(obj)/%.o: $(src)/%.c $(recordmcount_source) FORCE
>         $(call if_changed_rule,cc_o_c)
>         $(call cmd,force_checksrc)
> 
> -ifdef CONFIG_LTO_CLANG
> +ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
>  # Module .o files may contain LLVM bitcode, compile them into native code
>  # before ELF processing
> -quiet_cmd_cc_lto_link_modules = LTO [M] $@
> -cmd_cc_lto_link_modules =                                              \
> +quiet_cmd_cc_prelink_modules = LD [M]  $@
> +      cmd_cc_prelink_modules =                                         \
>         $(LD) $(ld_flags) -r -o $@                                      \
> -               $(shell [ -s $(@:.lto.o=.o.symversions) ] &&            \
> -                       echo -T $(@:.lto.o=.o.symversions))             \
> +               $(shell [ -s $(@:.prelink.o=.o.symversions) ] &&
>          \
> +                       echo -T $(@:.prelink.o=.o.symversions))         \
>                 --whole-archive $(filter-out FORCE,$^)                  \
>                 $(cmd_objtool)
> 


> @@ -469,7 +470,7 @@ $(obj)/lib.a: $(lib-y) FORCE
>  # Do not replace $(filter %.o,^) with $(real-prereqs). When a single object
>  # module is turned into a multi object module, $^ will contain header file
>  # dependencies recorded in the .*.cmd file.
> -ifdef CONFIG_LTO_CLANG
> +ifneq ($(CONFIG_LTO_CLANG)$(CONFIG_X86_KERNEL_IBT),)
>  quiet_cmd_link_multi-m = AR [M]  $@
>  cmd_link_multi-m =                                             \
>         $(cmd_update_lto_symversions);                          \

Except I overlooked this part, where ar is used instead of ld to combine
the individual files.

Let me go make the change, Thanks!

Re: [PATCH v4 00/45] x86: Kernel IBT

[Peter Zijlstra]

On Tue, Mar 15, 2022 at 11:26:10AM -0700, Alexei Starovoitov wrote:
> The bpf trampoline can attach to both indirect and non-indirect
> functions. My understanding is that only indirect targets will have
> endbr first insn. So the fix totally makes sense.

Correct, the compiler is free to not emit endbr if it can determine the
function will never be called indirectly (or it is explicitly marked so
with a function attribute).

RE: [PATCH v4 00/45] x86: Kernel IBT

[David Laight]

From: Peter Zijlstra
> Sent: 17 March 2022 19:45
> 
> On Wed, Mar 16, 2022 at 01:28:08AM +0900, Masahiro Yamada wrote:
> > On Tue, Mar 15, 2022 at 5:15 PM Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > Index: linux-2.6/scripts/Makefile.build
> > > ===================================================================
> > > --- linux-2.6.orig/scripts/Makefile.build
> > > +++ linux-2.6/scripts/Makefile.build
> > > @@ -86,12 +86,18 @@ ifdef need-builtin
> > >  targets-for-builtin += $(obj)/built-in.a
> > >  endif
> > >
> > > -targets-for-modules := $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> > > +targets-for-modules :=
> >
> >
> > Why do you need to change this line?
> >
> >
> >
> > >
> > >  ifdef CONFIG_LTO_CLANG
> > >  targets-for-modules += $(patsubst %.o, %.lto.o, $(filter %.o, $(obj-m)))
> > >  endif
> > >
> > > +ifdef CONFIG_X86_KERNEL_IBT
> > > +targets-for-modules += $(patsubst %.o, %.objtool, $(filter %.o, $(obj-m)))
> > > +endif
> > > +
> > > +targets-for-modules += $(patsubst %.o, %.mod, $(filter %.o, $(obj-m)))
> > > +
> > >  ifdef need-modorder
> > >  targets-for-modules += $(obj)/modules.order
> > >  endif
> 
> The thinking was that by having the .objtool rule before the .mod rule,
> objtool runs first. If mod runs before objtool, objtool will change the
> timestamp and then mod will get remade, even if nothing's changed.

I don't think it should make any difference.
A quick peruse didn't show where targets-for-modules actually
ends up being used (after being added to targets).
But in a makefile, if you have:

x: a b

nothing requires make to generate 'a' before or after 'b'.
gmake might have something similar to nmake's .ORDER directive
but I don't remember seeing it defined anywhere.
You can add 'b: a' to force the order (which is how .ORDER
ends up being implemented).
But I didn't spot anything of that nature.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)