Re: linux-next: build warnings after merge of the tip tree

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > Hi all,
> > 
> > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > produced these new warnings:
> > 
> > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> 
> Hurmph, lemme go figure out where that code comes from, I've not seen
> those.

Ahh, something tracing. I'll go do some patches on top of it.

Also, folks, I'm thinking we should start to move to __fexit__, if CET
SHSTK ever wants to come to kernel land return trampolines will
insta-stop working.

Hjl, do you think we could get -mfexit to go along with -mfentry ?

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > Hi all,
> > > 
> > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > produced these new warnings:
> > > 
> > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > 
> > Hurmph, lemme go figure out where that code comes from, I've not seen
> > those.
> 
> Ahh, something tracing. I'll go do some patches on top of it.

Also, that x86 patch has never his x86@kernel.org and doesn't have an
ACK from any x86 person :-(((

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 02:08:23PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > Hi all,
> > > > 
> > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > produced these new warnings:
> > > > 
> > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > 
> > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > those.
> > 
> > Ahh, something tracing. I'll go do some patches on top of it.
> 
> Also, that x86 patch has never his x86@kernel.org and doesn't have an
> ACK from any x86 person :-(((

Worse, it adds a 3rd return trampoline without replacing any of the
existing two :-(

Why was this merged?

Re: linux-next: build warnings after merge of the tip tree

[Mark Rutland]

On Mon, Mar 21, 2022 at 02:45:16PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 21, 2022 at 02:08:23PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > > Hi all,
> > > > > 
> > > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > > produced these new warnings:
> > > > > 
> > > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > 
> > > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > > those.
> > > 
> > > Ahh, something tracing. I'll go do some patches on top of it.
> > 
> > Also, that x86 patch has never his x86@kernel.org and doesn't have an
> > ACK from any x86 person :-(((
> 
> Worse, it adds a 3rd return trampoline without replacing any of the
> existing two :-(

Likewise; I have the same complaints for the arm64 patch.

I haven't had the chance to review/ack that, and I'm actively working on
improving out unwinder and the way it interacts with the various *existing*
trampolines, so adding yat another is *not* good.

> Why was this merged?

Likewise, same question for arm64?

Thanks,
Mark.

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 02:45:16PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 21, 2022 at 02:08:23PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > > Hi all,
> > > > > 
> > > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > > produced these new warnings:
> > > > > 
> > > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > 
> > > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > > those.
> > > 
> > > Ahh, something tracing. I'll go do some patches on top of it.
> > 
> > Also, that x86 patch has never his x86@kernel.org and doesn't have an
> > ACK from any x86 person :-(((
> 
> Worse, it adds a 3rd return trampoline without replacing any of the
> existing two :-(
> 
> Why was this merged?

It additionally gets ret wrong:

  vmlinux.o: warning: objtool: arch_rethook_trampoline()+0x4a: missing int3 after ret

and afaict regs->ss is garbage (much like kretprobes it appears).

Can we please unmerge this stuff and try again later?

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 04:28:06PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 21, 2022 at 02:45:16PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 02:08:23PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > > > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > > > produced these new warnings:
> > > > > > 
> > > > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > 
> > > > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > > > those.
> > > > 
> > > > Ahh, something tracing. I'll go do some patches on top of it.
> > > 
> > > Also, that x86 patch has never his x86@kernel.org and doesn't have an
> > > ACK from any x86 person :-(((
> > 
> > Worse, it adds a 3rd return trampoline without replacing any of the
> > existing two :-(
> > 
> > Why was this merged?
> 
> It additionally gets ret wrong:
> 
>   vmlinux.o: warning: objtool: arch_rethook_trampoline()+0x4a: missing int3 after ret
> 
> and afaict regs->ss is garbage (much like kretprobes it appears).
> 
> Can we please unmerge this stuff and try again later?

This landing in -next only today (after having been committed last
friday night) is another clue it should probably go next round.

Re: linux-next: build warnings after merge of the tip tree

[Linus Torvalds]

On Mon, Mar 21, 2022 at 8:46 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> This landing in -next only today (after having been committed last
> friday night) is another clue it should probably go next round.

I went and looked at lore to get the context that I didn't get in this
email that I was added to the participants for.

I didn't find the context there either.

Sure, I found the thread, but the whole "that x86 patch" that you
refer to was never actually specified even in the full thread as far
as I can tell. I see that there is an arm64 equivalent too of what you
are complaining about, and I have no idea about that one either..

Mind actually giving the full details so that we don't have to go
re-do the work you already did?

Because right now I know something is wrong, I know the warnings, but
I don't actually have any handle on the actual patches to look out
for.

It's presumably not in any of the pull requests I already have
pending, but it would be nice if I saw some details of _what_ you are
complaining about, and not just the complaint itself ;)

                     Linus

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 09:37:35AM -0700, Linus Torvalds wrote:
> On Mon, Mar 21, 2022 at 8:46 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > This landing in -next only today (after having been committed last
> > friday night) is another clue it should probably go next round.
> 
> I went and looked at lore to get the context that I didn't get in this
> email that I was added to the participants for.
> 
> I didn't find the context there either.
> 
> Sure, I found the thread, but the whole "that x86 patch" that you
> refer to was never actually specified even in the full thread as far
> as I can tell. I see that there is an arm64 equivalent too of what you
> are complaining about, and I have no idea about that one either..
> 
> Mind actually giving the full details so that we don't have to go
> re-do the work you already did?
> 
> Because right now I know something is wrong, I know the warnings, but
> I don't actually have any handle on the actual patches to look out
> for.
> 
> It's presumably not in any of the pull requests I already have
> pending, but it would be nice if I saw some details of _what_ you are
> complaining about, and not just the complaint itself ;)

Duh, right. It's this series:

  https://lore.kernel.org/bpf/164757541675.26179.17727138330733641017.git-patchwork-notify@kernel.org/

That went into bpf-next last Friday. I just checked but haven't found a
pull for it yet.

Re: linux-next: build warnings after merge of the tip tree

[Linus Torvalds]

On Mon, Mar 21, 2022 at 9:45 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > It's presumably not in any of the pull requests I already have
> > pending, but it would be nice if I saw some details of _what_ you are
> > complaining about, and not just the complaint itself ;)
>
> Duh, right. It's this series:
>
>   https://lore.kernel.org/bpf/164757541675.26179.17727138330733641017.git-patchwork-notify@kernel.org/
>
> That went into bpf-next last Friday. I just checked but haven't found a
> pull for it yet.

Thanks. I can confirm it's not in any of the pull requests I have
pending, so I'll just start doing my normal work and try to remember
to look out for this issue later.

                 Linus

Re: linux-next: build warnings after merge of the tip tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

Hi all,

On Mon, 21 Mar 2022 09:52:58 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> On Mon, Mar 21, 2022 at 9:45 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > It's presumably not in any of the pull requests I already have
> > > pending, but it would be nice if I saw some details of _what_ you are
> > > complaining about, and not just the complaint itself ;)  
> >
> > Duh, right. It's this series:
> >
> >   https://lore.kernel.org/bpf/164757541675.26179.17727138330733641017.git-patchwork-notify@kernel.org/
> >
> > That went into bpf-next last Friday. I just checked but haven't found a
> > pull for it yet.  
> 
> Thanks. I can confirm it's not in any of the pull requests I have
> pending, so I'll just start doing my normal work and try to remember
> to look out for this issue later.

The normal path for bpf-next code is via the net-next tree.  But the
above series has not yet been merged into the net-next tree so is only
in the bpf-next tree.

So, what am I to do?  Drop the bpf-next tree from linux-next until this
is resolved?  Some input from the BPF people would be useful.

Dave, Jakub, please do not merge the bpf-bext tree into the net-next
tree for now.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warnings after merge of the tip tree

[Alexei Starovoitov]

On Mon, Mar 21, 2022 at 3:05 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi all,
>
> On Mon, 21 Mar 2022 09:52:58 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> > On Mon, Mar 21, 2022 at 9:45 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > It's presumably not in any of the pull requests I already have
> > > > pending, but it would be nice if I saw some details of _what_ you are
> > > > complaining about, and not just the complaint itself ;)
> > >
> > > Duh, right. It's this series:
> > >
> > >   https://lore.kernel.org/bpf/164757541675.26179.17727138330733641017.git-patchwork-notify@kernel.org/
> > >
> > > That went into bpf-next last Friday. I just checked but haven't found a
> > > pull for it yet.
> >
> > Thanks. I can confirm it's not in any of the pull requests I have
> > pending, so I'll just start doing my normal work and try to remember
> > to look out for this issue later.
>
> The normal path for bpf-next code is via the net-next tree.  But the
> above series has not yet been merged into the net-next tree so is only
> in the bpf-next tree.
>
> So, what am I to do?  Drop the bpf-next tree from linux-next until this
> is resolved?  Some input from the BPF people would be useful.
>
> Dave, Jakub, please do not merge the bpf-bext tree into the net-next
> tree for now.

That makes little sense. It's not an unusual merge conflict.
Peter's endbr series conflict with Masami's fprobe.
Peter has a trivial patch that fixes objtool warning.
The question is how to land that patch.
I think the best is for Linus to apply it after bpf-next->net-next gets
merged.

We're preparing bpf-next PR right now.

Re: linux-next: build warnings after merge of the tip tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

Hi Alexei,

On Mon, 21 Mar 2022 15:12:05 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> That makes little sense. It's not an unusual merge conflict.
> Peter's endbr series conflict with Masami's fprobe.
> Peter has a trivial patch that fixes objtool warning.
> The question is how to land that patch.
> I think the best is for Linus to apply it after bpf-next->net-next gets
> merged.

Peter has other concerns, please read the thread and consider them.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: build warnings after merge of the tip tree

[Alexei Starovoitov]

On Mon, Mar 21, 2022 at 3:46 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi Alexei,
>
> On Mon, 21 Mar 2022 15:12:05 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> >
> > That makes little sense. It's not an unusual merge conflict.
> > Peter's endbr series conflict with Masami's fprobe.
> > Peter has a trivial patch that fixes objtool warning.
> > The question is how to land that patch.
> > I think the best is for Linus to apply it after bpf-next->net-next gets
> > merged.
>
> Peter has other concerns, please read the thread and consider them.

Masami is an expert in kprobe. He copy pasted a bit of kprobe logic
to make it into 'multi kprobe' (he calls it fprobe).
I believe he knows what he's doing.
Steven reviewed and tested that set.
We've tested it as well and don't have any correctness or api concerns.

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 15:50:17 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> > Peter has other concerns, please read the thread and consider them.  
> 
> Masami is an expert in kprobe. He copy pasted a bit of kprobe logic
> to make it into 'multi kprobe' (he calls it fprobe).
> I believe he knows what he's doing.
> Steven reviewed and tested that set.
> We've tested it as well and don't have any correctness or api concerns.

I tested it from a ftrace perspective, not an IBT or other work being done
in the x86 world.

I'm fine with the work being done in kernel/tracing/ but it still requires
the arch maintainer's acks for anything in arch/. I was under the
impression that the arch specific code was Cc'ing the arch maintainers
(which I always do when I touch their code). But I missed that they were
not. That's my fault, I should have caught that.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Mon, 21 Mar 2022 15:50:17 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Mon, Mar 21, 2022 at 3:46 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > Hi Alexei,
> >
> > On Mon, 21 Mar 2022 15:12:05 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > >
> > > That makes little sense. It's not an unusual merge conflict.
> > > Peter's endbr series conflict with Masami's fprobe.
> > > Peter has a trivial patch that fixes objtool warning.
> > > The question is how to land that patch.
> > > I think the best is for Linus to apply it after bpf-next->net-next gets
> > > merged.
> >
> > Peter has other concerns, please read the thread and consider them.
> 
> Masami is an expert in kprobe. He copy pasted a bit of kprobe logic
> to make it into 'multi kprobe' (he calls it fprobe).
> I believe he knows what he's doing.
> Steven reviewed and tested that set.
> We've tested it as well and don't have any correctness or api concerns.

Sorry, that's my mistake to not Ccing to arch maintainers for the arch
dependent patches. Let me update and send v13 for this fprobe series.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Alexei Starovoitov]

On Mon, Mar 21, 2022 at 9:51 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> On Mon, 21 Mar 2022 15:50:17 -0700
> Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>
> > On Mon, Mar 21, 2022 at 3:46 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > >
> > > Hi Alexei,
> > >
> > > On Mon, 21 Mar 2022 15:12:05 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > >
> > > > That makes little sense. It's not an unusual merge conflict.
> > > > Peter's endbr series conflict with Masami's fprobe.
> > > > Peter has a trivial patch that fixes objtool warning.
> > > > The question is how to land that patch.
> > > > I think the best is for Linus to apply it after bpf-next->net-next gets
> > > > merged.
> > >
> > > Peter has other concerns, please read the thread and consider them.
> >
> > Masami is an expert in kprobe. He copy pasted a bit of kprobe logic
> > to make it into 'multi kprobe' (he calls it fprobe).
> > I believe he knows what he's doing.
> > Steven reviewed and tested that set.
> > We've tested it as well and don't have any correctness or api concerns.
>
> Sorry, that's my mistake to not Ccing to arch maintainers for the arch
> dependent patches. Let me update and send v13 for this fprobe series.

No. Please read what I've said earlier.

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 03:12:05PM -0700, Alexei Starovoitov wrote:
> 
> That makes little sense. It's not an unusual merge conflict.

It is not only that; it is adding code that with an x86 arch maintainer
hat on I've never seen and don't agree with. Same for arm64 apparently.

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Mon, 21 Mar 2022 14:45:16 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Mar 21, 2022 at 02:08:23PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > > Hi all,
> > > > > 
> > > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > > produced these new warnings:
> > > > > 
> > > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > 
> > > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > > those.
> > > 
> > > Ahh, something tracing. I'll go do some patches on top of it.
> > 
> > Also, that x86 patch has never his x86@kernel.org and doesn't have an
> > ACK from any x86 person :-(((
> 
> Worse, it adds a 3rd return trampoline without replacing any of the
> existing two :-(

Sorry about this. I missed to add Ccing to arch maintainers.
Let me check how I can fix those errors.

Thanks.

> 
> Why was this merged?


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 14:04:05 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> Ahh, something tracing. I'll go do some patches on top of it.
> 
> Also, folks, I'm thinking we should start to move to __fexit__, if CET
> SHSTK ever wants to come to kernel land return trampolines will
> insta-stop working.
> 
> Hjl, do you think we could get -mfexit to go along with -mfentry ?

If we do every add a -mfexit, we will need to add a __ftail__ call.
Because, the current function exit tracing works for functions, even with
tail calls.


int funcA () {
	[..]
	return funcB();
}

Can turn into:

	[..]
	pop all stack from funcA
	load reg params to funcB
	jmp funcB

Then when funcB does does it's

	[..]
	ret

It will pop the call site of funcA (not the call site of funcB) and return
to wherever called funcA with the proper return values.

This currently works with function graph and kretprobe tracing because of
the shadow stack. Let's say we traced both funcA and funcB

funcA:
	call __fentry__

			Replace caller address with graph_trampoline and
			store the return caller into the shadow stack.

	[..]
	jmp funcB

funcB:
	call __fentry__

			Replace caller address with graph_trampoline and
			store the return caller (which is the
			graph_trampoline that was switched earlier) in the
			shadow stack.

	[..]
	ret

			Returns to the graph_trampoline and we trace the
			return of funcB. Then we pop off the shadow stack
			and jump to that. But the shadow stack had a call
			to the graph_trampoline, which gets called again.

			Returns to the graph_trampoline and we trace the
			return of funcA. Then we pop off the shadow stack
			and jump to that, which is the original caller to
			funcA.

That is, the current algorithm traces the end of both funcA and funcB
without issue, because of how the shadow stack works.

Now if we add a __fexit__, we will need a way to tell the tracers how to
record this scenario. That is why I'm thinking of a jmp to __ftail__.

Perhaps something like:

funcA:
	call __fentry__
	[..]
	push address of funcB
	jmp __ftail__
	jmp funcB

Where, __ftail__ would do at the end:

	ret

To jump to funcB and we skip the jmp to funcB anyway.

And to "nop" it out, we would have to convert it to.

funcA:
	call __fentry__
	[..]
	jmp 1
	jmp __ftail__
1:	jmp funcB


This is one way I can think of if we include a __fexit__. But to maintain
backward compatibility to function graph tracing (which is a requirement),
we need to be able to handle such cases.

Perhaps this is a good topic to bring up at Plumbers? :-)

Do I need to submit a tracing MC, or can we have this conversation at a
compiler / toolchain MC?

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 11:28:05AM -0400, Steven Rostedt wrote:
> On Mon, 21 Mar 2022 14:04:05 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:

> > Also, folks, I'm thinking we should start to move to __fexit__, if CET
> > SHSTK ever wants to come to kernel land return trampolines will
> > insta-stop working.
> > 
> > Hjl, do you think we could get -mfexit to go along with -mfentry ?

> int funcA () {
> 	[..]
> 	return funcB();
> }

> This currently works with function graph and kretprobe tracing because of
> the shadow stack. Let's say we traced both funcA and funcB
> 
> funcA:
> 	call __fentry__
			push funcA on trace-stack
> 
> 	[..]
> 	jmp funcB
> 
> funcB:
> 	call __fentry__
			push funcB on trace-stack
> 
> 	[..]
	call __fexit__
			pop trace-stack until empty
			  'exit funcB'
			  'exit funcA'

> 	ret

> 
> That is, the current algorithm traces the end of both funcA and funcB
> without issue, because of how the shadow stack works.

And it all works, no? Or what am I missing?

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 17:04:28 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Mar 21, 2022 at 11:28:05AM -0400, Steven Rostedt wrote:
> > On Mon, 21 Mar 2022 14:04:05 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:  
> 
> > > Also, folks, I'm thinking we should start to move to __fexit__, if CET
> > > SHSTK ever wants to come to kernel land return trampolines will
> > > insta-stop working.
> > > 
> > > Hjl, do you think we could get -mfexit to go along with -mfentry ?  
> 
> > int funcA () {
> > 	[..]
> > 	return funcB();
> > }  
> 
> > This currently works with function graph and kretprobe tracing because of
> > the shadow stack. Let's say we traced both funcA and funcB
> > 
> > funcA:
> > 	call __fentry__  
> 			push funcA on trace-stack
> > 
> > 	[..]
> > 	jmp funcB
> > 
> > funcB:
> > 	call __fentry__  
> 			push funcB on trace-stack
> > 
> > 	[..]  
> 	call __fexit__
> 			pop trace-stack until empty
> 			  'exit funcB'
> 			  'exit funcA'

And what happens if funcC called funcA and it too was on the stack. We pop
that too? But it's not done yet, because calling of funcA was not a tail
call.

-- Steve


> 
> > 	ret  
> 
> > 
> > That is, the current algorithm traces the end of both funcA and funcB
> > without issue, because of how the shadow stack works.  
> 
> And it all works, no? Or what am I missing?

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 12:12:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> > > funcB:
> > > 	call __fentry__    
> > 			push funcB on trace-stack  
> > > 
> > > 	[..]    
> > 	call __fexit__
> > 			pop trace-stack until empty
> > 			  'exit funcB'
> > 			  'exit funcA'  
> 
> And what happens if funcC called funcA and it too was on the stack. We pop
> that too? But it's not done yet, because calling of funcA was not a tail
> call.

And I just thought of another issue, where even my solution wont fix it.
What happens if we trace funcA but not funcB? How do we get to trace the
end of funcA?

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 12:15:49 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> And I just thought of another issue, where even my solution wont fix it.
> What happens if we trace funcA but not funcB? How do we get to trace the
> end of funcA?

The only solution I can think of to handle all these cases is if you enable
-mfexit, you have to disable tail calls completely. That's going to cause
a performance impact.

Perhaps we need need compiler help to give us a way to hijack the return
address. But is there a way to do this and still not give up the security
that CET SHSTK gives us?

Or maybe another solution is:

funcA:
	[..]
	jmp funcB
	call __fexit__
	ret

And if funcA is being traced, we change jmp to a call.

	[..]
	call funcB
	call __fexit__
	ret

Such that we only remove the tail calls if we enable tracing on the
function with the tail call.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 12:22:59 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Or maybe another solution is:
> 
> funcA:
> 	[..]
> 	jmp funcB
> 	call __fexit__
> 	ret
> 
> And if funcA is being traced, we change jmp to a call.
> 
> 	[..]
> 	call funcB
> 	call __fexit__

We could also make __fexit__ a tail call:

> 	ret


funcA:
	[..]
	call funcB
	jmp __fexit__

We would also need a way to know that funcA has a tail call at the end. So
more help from either the compiler or objtool.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 12:15:49PM -0400, Steven Rostedt wrote:
> On Mon, 21 Mar 2022 12:12:09 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > > funcB:
> > > > 	call __fentry__    
> > > 			push funcB on trace-stack  
> > > > 
> > > > 	[..]    
> > > 	call __fexit__
> > > 			pop trace-stack until empty
> > > 			  'exit funcB'
> > > 			  'exit funcA'  
> > 
> > And what happens if funcC called funcA and it too was on the stack. We pop
> > that too? But it's not done yet, because calling of funcA was not a tail
> > call.

Hmm, yeah, how about we have __ftail__ mark the left function.

func_B()
{
	...
}

func_A()
{
	...
	return func_B();
}

func_C()
{
	func_A();
	...
	return;
}

func_B:
	call __fentry__	/* push func_B */
	...
	call __fexit__	/* pop 1 + tails */
	ret

func_A:
	call __fentry__ /* push func_A */
	...
	call __ftail__  /* mark func_A tail */
	jmp func_B

func_C:
	call __fentry__ /* push func_C */
	call func_A;
	...
	call __fexit__  /* pop 1 + tails */
	ret;


Then the stack at the end of func_B looks something like:

	func_C
	func_A (tail)
	func_B

And it will pop func_B plus all tails (func_A).

> And I just thought of another issue, where even my solution wont fix it.
> What happens if we trace funcA but not funcB? How do we get to trace the
> end of funcA?

Disallow tail calls to notrace?

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 17:40:32 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> func_B:
> 	call __fentry__	/* push func_B */
> 	...
> 	call __fexit__	/* pop 1 + tails */
> 	ret
> 
> func_A:
> 	call __fentry__ /* push func_A */
> 	...
> 	call __ftail__  /* mark func_A tail */
> 	jmp func_B
> 
> func_C:
> 	call __fentry__ /* push func_C */
> 	call func_A;
> 	...
> 	call __fexit__  /* pop 1 + tails */
> 	ret;

This also assumes that we need to trace everything that is marked. I
mentioned in another email, what do we do if we only trace funcA?

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 12:45:51PM -0400, Steven Rostedt wrote:
> On Mon, 21 Mar 2022 17:40:32 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > func_B:
> > 	call __fentry__	/* push func_B */
> > 	...
> > 	call __fexit__	/* pop 1 + tails */
> > 	ret
> > 
> > func_A:
> > 	call __fentry__ /* push func_A */
> > 	...
> > 	call __ftail__  /* mark func_A tail */
> > 	jmp func_B
> > 
> > func_C:
> > 	call __fentry__ /* push func_C */
> > 	call func_A;
> > 	...
> > 	call __fexit__  /* pop 1 + tails */
> > 	ret;
> 
> This also assumes that we need to trace everything that is marked. I
> mentioned in another email, what do we do if we only trace funcA?

Like I said later on; if we inhibit tail-calls to notrace, this goes
away.

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Mon, 21 Mar 2022 17:50:50 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> > This also assumes that we need to trace everything that is marked. I
> > mentioned in another email, what do we do if we only trace funcA?  
> 
> Like I said later on; if we inhibit tail-calls to notrace, this goes
> away.

Please no. The number of "notrace" functions is increasing to the point
that it's starting to make function tracing useless in a lot of
circumstances. I've already lost my ability to see when user space goes
into the kernel (which I have to hack up custom coding to enable again).

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 12:54:19PM -0400, Steven Rostedt wrote:
> On Mon, 21 Mar 2022 17:50:50 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > This also assumes that we need to trace everything that is marked. I
> > > mentioned in another email, what do we do if we only trace funcA?  
> > 
> > Like I said later on; if we inhibit tail-calls to notrace, this goes
> > away.
> 
> Please no. The number of "notrace" functions is increasing to the point
> that it's starting to make function tracing useless in a lot of
> circumstances. I've already lost my ability to see when user space goes
> into the kernel (which I have to hack up custom coding to enable again).

I really can't follow the argument there, nor on IRC.

Suppose:

notrace func_B()
{
	...
}

func_A()
{
	...
	return func_B();
}

then inhibiting tail calls would end up looking like:

func_A:
	call __fentry__
	...
	call func_B
	call __fexit__
	ret

Then A is fully traced, B is invisible, as per spec. What is the
problem?

The problem you initially had, of doing a tail-call into a notrace, was
that the __fexit__ call went missing, because notrace will obviously not
have that. But that's avoided by inhibiting all tail-calls between
notrace and !notrace functions (note that notrace must also not
tail-call !notrace).

Your worry seems to stem about loosing visiblilty of !notrace functions,
but AFAICT that doesn't happen.

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Tue, 22 Mar 2022 08:54:55 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Mar 21, 2022 at 12:54:19PM -0400, Steven Rostedt wrote:
> > On Mon, 21 Mar 2022 17:50:50 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >   
> > > > This also assumes that we need to trace everything that is marked. I
> > > > mentioned in another email, what do we do if we only trace funcA?    
> > > 
> > > Like I said later on; if we inhibit tail-calls to notrace, this goes
> > > away.  
> > 
> > Please no. The number of "notrace" functions is increasing to the point
> > that it's starting to make function tracing useless in a lot of
> > circumstances. I've already lost my ability to see when user space goes
> > into the kernel (which I have to hack up custom coding to enable again).  
> 
> I really can't follow the argument there, nor on IRC.
> 
> Suppose:
> 
> notrace func_B()
> {
> 	...
> }
> 
> func_A()
> {
> 	...
> 	return func_B();
> }
> 
> then inhibiting tail calls would end up looking like:

If we inhibit tail calls, then we do not need to make func_B notrace.

> 
> func_A:
> 	call __fentry__
> 	...
> 	call func_B
> 	call __fexit__
> 	ret
> 
> Then A is fully traced, B is invisible, as per spec. What is the
> problem?

The above is fine, but then func_B is not a tail call and can also be
traced.

> 
> The problem you initially had, of doing a tail-call into a notrace, was
> that the __fexit__ call went missing, because notrace will obviously not
> have that. But that's avoided by inhibiting all tail-calls between
> notrace and !notrace functions (note that notrace must also not
> tail-call !notrace).

I'm confused by the above. Why can't a notrace tail call a !notrace?
If we tail call to a

func_B:
	call __fentry__
	...
	call __fexit__
	ret

then the fentry and fexit show a perfectly valid trace of func_B.


> 
> Your worry seems to stem about loosing visiblilty of !notrace functions,
> but AFAICT that doesn't happen.

My worry is:

func_A:
	call __fentry__
	...
	jmp func_B

Where do we do the call __fexit__ ?

That was the original concern, and I think the proposed solutions have
convoluted our thoughts about what we are trying to fix. So let's go back
to the beginning, and see how to deal with it.

That is, we have:

func_C:
	call __fenty__
	...
	call func_A:
	...
	call func_B:
	...
	call __fexit__
	ret

func_A:
	call __fentry__
	...
	jmp func_B

func_B:
	call __fentry__
	...
	call __fexit__
	ret

Where the above is C calling A and B as normal functions, A calling B as a
tail call and B just being a normal function called by both A and C (and
many other functions).

And note, I do not want to limit function tracing (which does not rely on
__fexit__) just because we can't figure out how to handle __fexit__.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Tue, Mar 22, 2022 at 09:12:42AM -0400, Steven Rostedt wrote:

> > Suppose:
> > 
> > notrace func_B()
> > {
> > 	...
> > }
> > 
> > func_A()
> > {
> > 	...
> > 	return func_B();
> > }
> > 
> > then inhibiting tail calls would end up looking like:
> 
> If we inhibit tail calls, then we do not need to make func_B notrace.

Dude, you're arguing in circles :-( the notrace was a given.

> > func_A:
> > 	call __fentry__
> > 	...
> > 	call func_B
> > 	call __fexit__
> > 	ret
> > 
> > Then A is fully traced, B is invisible, as per spec. What is the
> > problem?
> 
> The above is fine, but then func_B is not a tail call and can also be
> traced.

Again, B is notrace as a given. This was all about how to deal with
notrace functions.

I suggested inhibiting tail-call to notrace, you said no. You now seem to
agree that solves it.

> > The problem you initially had, of doing a tail-call into a notrace, was
> > that the __fexit__ call went missing, because notrace will obviously not
> > have that. But that's avoided by inhibiting all tail-calls between
> > notrace and !notrace functions (note that notrace must also not
> > tail-call !notrace).
> 
> I'm confused by the above. Why can't a notrace tail call a !notrace?
> If we tail call to a
> 
> func_B:
> 	call __fentry__
> 	...
> 	call __fexit__
> 	ret
> 
> then the fentry and fexit show a perfectly valid trace of func_B.

Bah; I thought I had a case this morning, but now I can't seem to recall
:/

> > Your worry seems to stem about loosing visiblilty of !notrace functions,
> > but AFAICT that doesn't happen.
> 
> My worry is:
> 
> func_A:
> 	call __fentry__
> 	...
> 	jmp func_B
> 
> Where do we do the call __fexit__ ?

In B (or wherever if B again does a tail-call).

> That was the original concern, and I think the proposed solutions have
> convoluted our thoughts about what we are trying to fix. So let's go back
> to the beginning, and see how to deal with it.
> 
> That is, we have:
> 
> func_C:
> 	call __fenty__
> 	...
> 	call func_A:
> 	...
> 	call func_B:
> 	...
> 	call __fexit__
> 	ret
> 
> func_A:
> 	call __fentry__
> 	...
	call __ftail__
> 	jmp func_B
> 
> func_B:
> 	call __fentry__
> 	...
> 	call __fexit__
> 	ret
> 
> Where the above is C calling A and B as normal functions, A calling B as a
> tail call and B just being a normal function called by both A and C (and
> many other functions).

We need the __ftail__ thing to mark the trace-stack entry of func_A as
complete, then any future __fexit__ will be able to pop all completed
entries.

In recap:

	__fentry__ -- push on trace-stack
	__ftail__  -- mark top-most entry complete
	__fexit__  -- mark top-most entry complete;
	              pop all completed entries

inhibit tail-calls to notrace.

> And note, I do not want to limit function tracing (which does not rely on
> __fexit__) just because we can't figure out how to handle __fexit__.

I'm not following. Regular function tracing needs none of this.

It's function graph tracing, kretprobes and whatever else this rethook
stuff is about that needs this because return trampolines will stop
working somewhere in the not too distant future.

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Tue, 22 Mar 2022 15:35:54 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Mar 22, 2022 at 09:12:42AM -0400, Steven Rostedt wrote:
> 
> > > Suppose:
> > > 
> > > notrace func_B()
> > > {
> > > 	...
> > > }
> > > 
> > > func_A()
> > > {
> > > 	...
> > > 	return func_B();
> > > }
> > > 
> > > then inhibiting tail calls would end up looking like:  
> > 
> > If we inhibit tail calls, then we do not need to make func_B notrace.  
> 
> Dude, you're arguing in circles :-( the notrace was a given.

Why is it notrace? Sorry to be dense, but it's not a given to me.

> 
> > > func_A:
> > > 	call __fentry__
> > > 	...
> > > 	call func_B
> > > 	call __fexit__
> > > 	ret
> > > 
> > > Then A is fully traced, B is invisible, as per spec. What is the
> > > problem?  
> > 
> > The above is fine, but then func_B is not a tail call and can also be
> > traced.  
> 
> Again, B is notrace as a given. This was all about how to deal with
> notrace functions.


No, it was about how to deal with notrace functions that were tail calls.
That was what I had an issue with. Because the solution you proposed had
func_A depend on func_B executing its call __fexit__ which it would not, if
it was not traced.

> 
> I suggested inhibiting tail-call to notrace, you said no. You now seem to
> agree that solves it.

I said inhibiting tail-calls was a solution, but only inhibiting it to
notrace would probably have a significant performance impact.

I thought you were talking about adding notrace to tail calls, not the
other way around. Maybe that is our confusion in this conversation.

> 
> > > The problem you initially had, of doing a tail-call into a notrace, was
> > > that the __fexit__ call went missing, because notrace will obviously not
> > > have that. But that's avoided by inhibiting all tail-calls between
> > > notrace and !notrace functions (note that notrace must also not
> > > tail-call !notrace).  
> > 
> > I'm confused by the above. Why can't a notrace tail call a !notrace?
> > If we tail call to a
> > 
> > func_B:
> > 	call __fentry__
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > then the fentry and fexit show a perfectly valid trace of func_B.  
> 
> Bah; I thought I had a case this morning, but now I can't seem to recall
> :/

That happens to all of us ;-)

> 
> > > Your worry seems to stem about loosing visiblilty of !notrace functions,
> > > but AFAICT that doesn't happen.  
> > 
> > My worry is:
> > 
> > func_A:
> > 	call __fentry__
> > 	...
> > 	jmp func_B
> > 
> > Where do we do the call __fexit__ ?  
> 
> In B (or wherever if B again does a tail-call).

But there's no guarantee that the call __fexit__ will not be a nop in
func_B. Remember, these are all turned on and off. If we just trace func_A
and not func_B, we will never have a call __fexit__ for func_A.

> 
> > That was the original concern, and I think the proposed solutions have
> > convoluted our thoughts about what we are trying to fix. So let's go back
> > to the beginning, and see how to deal with it.
> > 
> > That is, we have:
> > 
> > func_C:
> > 	call __fenty__
> > 	...
> > 	call func_A:
> > 	...
> > 	call func_B:
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > func_A:
> > 	call __fentry__
> > 	...  
> 	call __ftail__
> > 	jmp func_B
> > 
> > func_B:
> > 	call __fentry__
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > Where the above is C calling A and B as normal functions, A calling B as a
> > tail call and B just being a normal function called by both A and C (and
> > many other functions).  
> 
> We need the __ftail__ thing to mark the trace-stack entry of func_A as
> complete, then any future __fexit__ will be able to pop all completed
> entries.
> 
> In recap:
> 
> 	__fentry__ -- push on trace-stack
> 	__ftail__  -- mark top-most entry complete
> 	__fexit__  -- mark top-most entry complete;
> 	              pop all completed entries

Again, this would require that the tail-calls are also being traced.

> 
> inhibit tail-calls to notrace.

Just inhibiting tail-calls to notrace would work without any of the above.
But my fear is that will cause a noticeable performance impact.

> 
> > And note, I do not want to limit function tracing (which does not rely on
> > __fexit__) just because we can't figure out how to handle __fexit__.  
> 
> I'm not following. Regular function tracing needs none of this.

The regular function tracing does not need this. Only function graph
tracing. I was thinking you were *adding* notrace to tail calls and such,
which is what I was against. But apparently that is not what you were
saying.

> 
> It's function graph tracing, kretprobes and whatever else this rethook
> stuff is about that needs this because return trampolines will stop
> working somewhere in the not too distant future.

Another crazy solution is to have:

func_A:
	call __fentry__
	...
tail:	jmp 1f 
	call 1f
	call __fexit__
	ret
1:	jmp func_B


where the compiler tells us about "tail:" and that we know that func_A ends
with a tail call, and if we want to trace the end of func_A we convert that
jmp 1f into a nop. And then we call the func_B and it's return comes back
to where we call __fexit__ and then return normally.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Tue, Mar 22, 2022 at 11:04:38AM -0400, Steven Rostedt wrote:
> On Tue, 22 Mar 2022 15:35:54 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:

> > I suggested inhibiting tail-call to notrace, you said no. You now seem to
> > agree that solves it.
> 
> I said inhibiting tail-calls was a solution, but only inhibiting it to
> notrace would probably have a significant performance impact.
> 
> I thought you were talking about adding notrace to tail calls, not the
> other way around. Maybe that is our confusion in this conversation.

Yeah, I meant inhibiting the compiler from doing tail-calls.

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Tue, Mar 22, 2022 at 11:04:38AM -0400, Steven Rostedt wrote:

> > In recap:
> > 
> > 	__fentry__ -- push on trace-stack
> > 	__ftail__  -- mark top-most entry complete
> > 	__fexit__  -- mark top-most entry complete;
> > 	              pop all completed entries
> 
> Again, this would require that the tail-calls are also being traced.

Which is why we should inhibit tail-calls if the function is notrace.

> > inhibit tail-calls to notrace.
> 
> Just inhibiting tail-calls to notrace would work without any of the above.

I'm lost again; what? Without any of the above you got nothing because
return-trampoline will not work.

> But my fear is that will cause a noticeable performance impact.

Most code isn't in fact notrace, and call+ret aren't *that* expensive.

> > It's function graph tracing, kretprobes and whatever else this rethook
> > stuff is about that needs this because return trampolines will stop
> > working somewhere in the not too distant future.
> 
> Another crazy solution is to have:
> 
> func_A:
> 	call __fentry__
> 	...
> tail:	jmp 1f 
> 	call 1f
	
> 	call __fexit__
> 	ret
> 1:	jmp func_B
> 
> 
> where the compiler tells us about "tail:" and that we know that func_A ends
> with a tail call, and if we want to trace the end of func_A we convert that
> jmp 1f into a nop. And then we call the func_B and it's return comes back
> to where we call __fexit__ and then return normally.

At that point giving us something like:

1:
	pushsection __ftail_loc
	.long	1b - .
	popsection

	jmp.d32	func_B
	call	__fexit__
	ret

is smaller and simpler, we can patch the jmp.d32 to call when tracing.
The only problem is SLS, that might wants an int3 after jmp too
( https://www.amd.com/en/corporate/product-security/bulletin/amd-sb-1026 ).

That does avoid the need for __ftail__ I suppose.

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Tue, 22 Mar 2022 16:48:02 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Mar 22, 2022 at 11:04:38AM -0400, Steven Rostedt wrote:
> 
> > > In recap:
> > > 
> > > 	__fentry__ -- push on trace-stack
> > > 	__ftail__  -- mark top-most entry complete
> > > 	__fexit__  -- mark top-most entry complete;
> > > 	              pop all completed entries  
> > 
> > Again, this would require that the tail-calls are also being traced.  
> 
> Which is why we should inhibit tail-calls if the function is notrace.
> 
> > > inhibit tail-calls to notrace.  
> > 
> > Just inhibiting tail-calls to notrace would work without any of the above.  
> 
> I'm lost again; what? Without any of the above you got nothing because
> return-trampoline will not work.


I think this got "lost in translation".

 "Inhibiting tail-calls to notrace"

Is a bit ambiguous because of the "to notrace" which would be different if
I had said "on notrace" which I may have screwed up the grammar here. Let
me be more precise.

 "Limiting tail-calls to only notrace functions"

That I think is a bit less ambiguous. English sucks.

> 
> > But my fear is that will cause a noticeable performance impact.  
> 
> Most code isn't in fact notrace, and call+ret aren't *that* expensive.

  "isn't in fact notrace" Ug! Double negatives!

This gets even more confusing when we are saying "notrace" which is a
negative. We should probably just say "traced" functions which makes
communication a bit more straight forward.

> 
> > > It's function graph tracing, kretprobes and whatever else this rethook
> > > stuff is about that needs this because return trampolines will stop
> > > working somewhere in the not too distant future.  
> > 
> > Another crazy solution is to have:
> > 
> > func_A:
> > 	call __fentry__
> > 	...
> > tail:	jmp 1f 
> > 	call 1f  
> 	
> > 	call __fexit__
> > 	ret
> > 1:	jmp func_B
> > 
> > 
> > where the compiler tells us about "tail:" and that we know that func_A ends
> > with a tail call, and if we want to trace the end of func_A we convert that
> > jmp 1f into a nop. And then we call the func_B and it's return comes back
> > to where we call __fexit__ and then return normally.  
> 
> At that point giving us something like:
> 
> 1:
> 	pushsection __ftail_loc
> 	.long	1b - .
> 	popsection
> 
> 	jmp.d32	func_B
> 	call	__fexit__
> 	ret
> 
> is smaller and simpler, we can patch the jmp.d32 to call when tracing.
> The only problem is SLS, that might wants an int3 after jmp too
> ( https://www.amd.com/en/corporate/product-security/bulletin/amd-sb-1026 ).
> 
> That does avoid the need for __ftail__ I suppose.

Which is basically what I said earlier ;-)

  https://lore.kernel.org/all/20220321122259.28146a7a@gandalf.local.home/

> Or maybe another solution is:
> 
> funcA:
> 	[..]
> 	jmp funcB
> 	call __fexit__
> 	ret
> 
> And if funcA is being traced, we change jmp to a call.
> 
> 	[..]
> 	call funcB
> 	call __fexit__
> 	ret
> 
> Such that we only remove the tail calls if we enable tracing on the
> function with the tail call.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Tue, 22 Mar 2022 15:35:54 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Mar 22, 2022 at 09:12:42AM -0400, Steven Rostedt wrote:
> 
> > > Suppose:
> > > 
> > > notrace func_B()
> > > {
> > > 	...
> > > }
> > > 
> > > func_A()
> > > {
> > > 	...
> > > 	return func_B();
> > > }
> > > 
> > > then inhibiting tail calls would end up looking like:
> > 
> > If we inhibit tail calls, then we do not need to make func_B notrace.
> 
> Dude, you're arguing in circles :-( the notrace was a given.
> 
> > > func_A:
> > > 	call __fentry__
> > > 	...
> > > 	call func_B
> > > 	call __fexit__
> > > 	ret
> > > 
> > > Then A is fully traced, B is invisible, as per spec. What is the
> > > problem?
> > 
> > The above is fine, but then func_B is not a tail call and can also be
> > traced.
> 
> Again, B is notrace as a given. This was all about how to deal with
> notrace functions.
> 
> I suggested inhibiting tail-call to notrace, you said no. You now seem to
> agree that solves it.
> 
> > > The problem you initially had, of doing a tail-call into a notrace, was
> > > that the __fexit__ call went missing, because notrace will obviously not
> > > have that. But that's avoided by inhibiting all tail-calls between
> > > notrace and !notrace functions (note that notrace must also not
> > > tail-call !notrace).
> > 
> > I'm confused by the above. Why can't a notrace tail call a !notrace?
> > If we tail call to a
> > 
> > func_B:
> > 	call __fentry__
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > then the fentry and fexit show a perfectly valid trace of func_B.
> 
> Bah; I thought I had a case this morning, but now I can't seem to recall
> :/
> 
> > > Your worry seems to stem about loosing visiblilty of !notrace functions,
> > > but AFAICT that doesn't happen.
> > 
> > My worry is:
> > 
> > func_A:
> > 	call __fentry__
> > 	...
> > 	jmp func_B
> > 
> > Where do we do the call __fexit__ ?
> 
> In B (or wherever if B again does a tail-call).
> 
> > That was the original concern, and I think the proposed solutions have
> > convoluted our thoughts about what we are trying to fix. So let's go back
> > to the beginning, and see how to deal with it.
> > 
> > That is, we have:
> > 
> > func_C:
> > 	call __fenty__
> > 	...
> > 	call func_A:
> > 	...
> > 	call func_B:
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > func_A:
> > 	call __fentry__
> > 	...
> 	call __ftail__
> > 	jmp func_B
> > 
> > func_B:
> > 	call __fentry__
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > Where the above is C calling A and B as normal functions, A calling B as a
> > tail call and B just being a normal function called by both A and C (and
> > many other functions).
> 
> We need the __ftail__ thing to mark the trace-stack entry of func_A as
> complete, then any future __fexit__ will be able to pop all completed
> entries.
> 
> In recap:
> 
> 	__fentry__ -- push on trace-stack
> 	__ftail__  -- mark top-most entry complete
> 	__fexit__  -- mark top-most entry complete;
> 	              pop all completed entries
> 
> inhibit tail-calls to notrace.
> 
> > And note, I do not want to limit function tracing (which does not rely on
> > __fexit__) just because we can't figure out how to handle __fexit__.
> 
> I'm not following. Regular function tracing needs none of this.
> 
> It's function graph tracing, kretprobes and whatever else this rethook
> stuff is about that needs this because return trampolines will stop
> working somewhere in the not too distant future.

I see the __fexit__ is needed, but why __ftail__ is needed? I guess because
func_B is notrace, in that case the __fexit__ will not be in the func_B.
Am I correct?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Wed, 23 Mar 2022 11:23:23 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> I see the __fexit__ is needed, but why __ftail__ is needed? I guess because
> func_B is notrace, in that case the __fexit__ will not be in the func_B.
> Am I correct?

I believe Peter and I agreed that the "best" solution so far, that has the
least amount of regressions (doesn't remove anything currently being
function graph traced, nor removes current tail calls) is:

> At that point giving us something like:
> 
> 1:
> 	pushsection __ftail_loc
> 	.long	1b - .
> 	popsection
> 
> 	jmp.d32	func_B
> 	call	__fexit__
> 	ret


Functions with a tail call will not have a __fexit__ and we can not rely on
the function that is the tail call to do the __fexit__ for the parent
function. Thus, the compromise is to add a label where the jmp to the
tail-call function is, and when we want to trace the return of that
function, we first have to patch the jmp into a call, which will then
return back to the call to __fexit__.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Tue, 22 Mar 2022 22:42:36 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 23 Mar 2022 11:23:23 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > I see the __fexit__ is needed, but why __ftail__ is needed? I guess because
> > func_B is notrace, in that case the __fexit__ will not be in the func_B.
> > Am I correct?
> 
> I believe Peter and I agreed that the "best" solution so far, that has the
> least amount of regressions (doesn't remove anything currently being
> function graph traced, nor removes current tail calls) is:
> 
> > At that point giving us something like:
> > 
> > 1:
> > 	pushsection __ftail_loc
> > 	.long	1b - .
> > 	popsection
> > 
> > 	jmp.d32	func_B
> > 	call	__fexit__
> > 	ret
> 
> 
> Functions with a tail call will not have a __fexit__ and we can not rely on
> the function that is the tail call to do the __fexit__ for the parent
> function. Thus, the compromise is to add a label where the jmp to the
> tail-call function is, and when we want to trace the return of that
> function, we first have to patch the jmp into a call, which will then
> return back to the call to __fexit__.

Got it. So the tail call "jump" will be replaced with a normal call when
we trace it.

That's a good idea :) 


> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Mon, 21 Mar 2022 12:12:09 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 21 Mar 2022 17:04:28 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, Mar 21, 2022 at 11:28:05AM -0400, Steven Rostedt wrote:
> > > On Mon, 21 Mar 2022 14:04:05 +0100
> > > Peter Zijlstra <peterz@infradead.org> wrote:  
> > 
> > > > Also, folks, I'm thinking we should start to move to __fexit__, if CET
> > > > SHSTK ever wants to come to kernel land return trampolines will
> > > > insta-stop working.
> > > > 
> > > > Hjl, do you think we could get -mfexit to go along with -mfentry ?  
> > 
> > > int funcA () {
> > > 	[..]
> > > 	return funcB();
> > > }  
> > 
> > > This currently works with function graph and kretprobe tracing because of
> > > the shadow stack. Let's say we traced both funcA and funcB
> > > 
> > > funcA:
> > > 	call __fentry__  
> > 			push funcA on trace-stack
> > > 
> > > 	[..]
> > > 	jmp funcB
> > > 
> > > funcB:
> > > 	call __fentry__  
> > 			push funcB on trace-stack
> > > 
> > > 	[..]  
> > 	call __fexit__
> > 			pop trace-stack until empty

This seems wrong. We don't pop the trace-stack until empty, but we will
record the real stack pointer at funcA.

> > 			  'exit funcB'
> > 			  'exit funcA'
> 
> And what happens if funcC called funcA and it too was on the stack. We pop
> that too? But it's not done yet, because calling of funcA was not a tail
> call.

Thus when the funcC is called, the trace-stack will be poped until funcA,
because we can see the real stack pointer at the 'ret'.
So the funcC is still on the trace-stack after that.

Thank you,


> 
> -- Steve
> 
> 
> > 
> > > 	ret  
> > 
> > > 
> > > That is, the current algorithm traces the end of both funcA and funcB
> > > without issue, because of how the shadow stack works.  
> > 
> > And it all works, no? Or what am I missing?
> 
> 
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > Hi all,
> > > 
> > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > produced these new warnings:
> > > 
> > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > 
> > Hurmph, lemme go figure out where that code comes from, I've not seen
> > those.
> 
> Ahh, something tracing. I'll go do some patches on top of it.

The below gets rid of the objtool warnings.

But I still think it's fairly terrible to get a (flawed) carbon copy of
the kretprobe code. Also, I think both should fix regs->ss.

---
diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
index f0f2f0608282..227a1890a984 100644
--- a/arch/x86/kernel/rethook.c
+++ b/arch/x86/kernel/rethook.c
@@ -20,6 +20,7 @@ asm(
 	".type arch_rethook_trampoline, @function\n"
 	"arch_rethook_trampoline:\n"
 #ifdef CONFIG_X86_64
+	ANNOTATE_NOENDBR
 	/* Push a fake return address to tell the unwinder it's a kretprobe. */
 	"	pushq $arch_rethook_trampoline\n"
 	UNWIND_HINT_FUNC
@@ -48,7 +49,7 @@ asm(
 	"	addl $4, %esp\n"
 	"	popfl\n"
 #endif
-	"	ret\n"
+	ASM_RET
 	".size arch_rethook_trampoline, .-arch_rethook_trampoline\n"
 );
 NOKPROBE_SYMBOL(arch_rethook_trampoline);

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Mon, 21 Mar 2022 17:48:54 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > Hi all,
> > > > 
> > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > produced these new warnings:
> > > > 
> > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > 
> > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > those.
> > 
> > Ahh, something tracing. I'll go do some patches on top of it.
> 
> The below gets rid of the objtool warnings.

Yes, I confirmed that.

> 
> But I still think it's fairly terrible to get a (flawed) carbon copy of
> the kretprobe code.

Indeed. I would like to replace the trampoline code of kretprobe with
rethook, eventually. There is no reason why we keep the clone.
(But I need more arch maintainers help for that, there are too many
 archs implemented kretprobes)

> Also, I think both should fix regs->ss.

I'm not sure this part. Since the return trampoline should run in the same
context of the called function, isn't ss same there too?

Thank you,

> 
> ---
> diff --git a/arch/x86/kernel/rethook.c b/arch/x86/kernel/rethook.c
> index f0f2f0608282..227a1890a984 100644
> --- a/arch/x86/kernel/rethook.c
> +++ b/arch/x86/kernel/rethook.c
> @@ -20,6 +20,7 @@ asm(
>  	".type arch_rethook_trampoline, @function\n"
>  	"arch_rethook_trampoline:\n"
>  #ifdef CONFIG_X86_64
> +	ANNOTATE_NOENDBR
>  	/* Push a fake return address to tell the unwinder it's a kretprobe. */
>  	"	pushq $arch_rethook_trampoline\n"
>  	UNWIND_HINT_FUNC
> @@ -48,7 +49,7 @@ asm(
>  	"	addl $4, %esp\n"
>  	"	popfl\n"
>  #endif
> -	"	ret\n"
> +	ASM_RET
>  	".size arch_rethook_trampoline, .-arch_rethook_trampoline\n"
>  );
>  NOKPROBE_SYMBOL(arch_rethook_trampoline);


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Tue, Mar 22, 2022 at 02:31:36PM +0900, Masami Hiramatsu wrote:

> > Also, I think both should fix regs->ss.
> 
> I'm not sure this part. Since the return trampoline should run in the same
> context of the called function, isn't ss same there too?

It creates pt_regs on the stack, so the trampolines do:

	push $arch_rethook_trampoline
	push %rsp
	pushf
	sub $24, %rsp /* cs, ip, orig_ax */
	push %rdi
	...
	push %r15

That means that if anybody looks at regs->ss, it'll find
$arch_rethook_trampoline, which isn't a valid segment descriptor, or am
I just really bad at counting today?

I'm thinking you want a copy of __KERNEL_DS in that stack slot, not a
function pointer.

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Tue, 22 Mar 2022 09:08:22 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Mar 22, 2022 at 02:31:36PM +0900, Masami Hiramatsu wrote:
> 
> > > Also, I think both should fix regs->ss.
> > 
> > I'm not sure this part. Since the return trampoline should run in the same
> > context of the called function, isn't ss same there too?
> 
> It creates pt_regs on the stack, so the trampolines do:
> 
> 	push $arch_rethook_trampoline
> 	push %rsp
> 	pushf
> 	sub $24, %rsp /* cs, ip, orig_ax */
> 	push %rdi
> 	...
> 	push %r15
> 
> That means that if anybody looks at regs->ss, it'll find
> $arch_rethook_trampoline, which isn't a valid segment descriptor, or am
> I just really bad at counting today?

Ah, got it. It seems that the ss was skipped from the beginning, and
no one argued that.

> I'm thinking you want a copy of __KERNEL_DS in that stack slot, not a
> function pointer.

The function pointer is for unwinding stack which involves the kretprobe.
Anyway, I can add a slot for ss if it is neeeded. But if it always be
__KERNEL_DS, is it worth to save it?

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Tue, Mar 22, 2022 at 06:14:54PM +0900, Masami Hiramatsu wrote:
> On Tue, 22 Mar 2022 09:08:22 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Tue, Mar 22, 2022 at 02:31:36PM +0900, Masami Hiramatsu wrote:
> > 
> > > > Also, I think both should fix regs->ss.
> > > 
> > > I'm not sure this part. Since the return trampoline should run in the same
> > > context of the called function, isn't ss same there too?
> > 
> > It creates pt_regs on the stack, so the trampolines do:
> > 
> > 	push $arch_rethook_trampoline
> > 	push %rsp
> > 	pushf
> > 	sub $24, %rsp /* cs, ip, orig_ax */
> > 	push %rdi
> > 	...
> > 	push %r15
> > 
> > That means that if anybody looks at regs->ss, it'll find
> > $arch_rethook_trampoline, which isn't a valid segment descriptor, or am
> > I just really bad at counting today?
> 
> Ah, got it. It seems that the ss was skipped from the beginning, and
> no one argued that.

Yeah, this is a long-standing issue, but I noticed it when looking at
the code yesterday.

> > I'm thinking you want a copy of __KERNEL_DS in that stack slot, not a
> > function pointer.
> 
> The function pointer is for unwinding stack which involves the kretprobe.
> Anyway, I can add a slot for ss if it is neeeded. But if it always be
> __KERNEL_DS, is it worth to save it?

Probably, to save someone future head-aches. The insn-eval.c stuff will
actually look at SS when it tries to decode BP/SP fields, and I've got
vague memories of actually using that a while ago. I think I was playing
around with double-fault and the whole espfix64 mess and hit the ESPFIX
segment.

Re: linux-next: build warnings after merge of the tip tree

[Peter Zijlstra]

On Tue, Mar 22, 2022 at 02:31:36PM +0900, Masami Hiramatsu wrote:

> > But I still think it's fairly terrible to get a (flawed) carbon copy of
> > the kretprobe code.
> 
> Indeed. I would like to replace the trampoline code of kretprobe with
> rethook, eventually. There is no reason why we keep the clone.
> (But I need more arch maintainers help for that, there are too many
>  archs implemented kretprobes)

CONFIG_KPROBE_ON_RETHOOK - and then implement archs one by one?

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Tue, 22 Mar 2022 13:17:18 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Mar 22, 2022 at 02:31:36PM +0900, Masami Hiramatsu wrote:
> 
> > > But I still think it's fairly terrible to get a (flawed) carbon copy of
> > > the kretprobe code.
> > 
> > Indeed. I would like to replace the trampoline code of kretprobe with
> > rethook, eventually. There is no reason why we keep the clone.
> > (But I need more arch maintainers help for that, there are too many
> >  archs implemented kretprobes)
> 
> CONFIG_KPROBE_ON_RETHOOK - and then implement archs one by one?

Sounds good! Maybe we will see different data structure fields
which depends on that config, but those are internal fields, so
user will not access it.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: linux-next: build warnings after merge of the tip tree

[Steven Rostedt]

On Tue, 22 Mar 2022 21:46:29 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > Indeed. I would like to replace the trampoline code of kretprobe with
> > > rethook, eventually. There is no reason why we keep the clone.
> > > (But I need more arch maintainers help for that, there are too many
> > >  archs implemented kretprobes)  
> > 
> > CONFIG_KPROBE_ON_RETHOOK - and then implement archs one by one?  
> 
> Sounds good! Maybe we will see different data structure fields
> which depends on that config, but those are internal fields, so
> user will not access it.

Which is basically what I do for ftrace. Which is why we have all these:

        select HAVE_DYNAMIC_FTRACE
        select HAVE_DYNAMIC_FTRACE_WITH_REGS
        select HAVE_DYNAMIC_FTRACE_WITH_ARGS
        select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
        select HAVE_SAMPLE_FTRACE_DIRECT
        select HAVE_SAMPLE_FTRACE_DIRECT_MULTI

in the architecture Kconfigs.

-- Steve

Re: linux-next: build warnings after merge of the tip tree

[Mark Rutland]

On Tue, Mar 22, 2022 at 02:31:36PM +0900, Masami Hiramatsu wrote:
> On Mon, 21 Mar 2022 17:48:54 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > > Hi all,
> > > > > 
> > > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > > produced these new warnings:
> > > > > 
> > > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > 
> > > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > > those.
> > > 
> > > Ahh, something tracing. I'll go do some patches on top of it.
> > 
> > The below gets rid of the objtool warnings.
> 
> Yes, I confirmed that.
> 
> > But I still think it's fairly terrible to get a (flawed) carbon copy of
> > the kretprobe code.
> 
> Indeed. I would like to replace the trampoline code of kretprobe with
> rethook, eventually. There is no reason why we keep the clone.
> (But I need more arch maintainers help for that, there are too many
>  archs implemented kretprobes)

FWIW, I'm more than happy to help on the arm64 side if you could Cc me for
that; I'm aware of other things in this area I'd like to clean up for
backtracing, too.

Thanks,
Mark.

Re: linux-next: build warnings after merge of the tip tree

[Masami Hiramatsu]

On Tue, 22 Mar 2022 13:15:58 +0000
Mark Rutland <mark.rutland@arm.com> wrote:

> On Tue, Mar 22, 2022 at 02:31:36PM +0900, Masami Hiramatsu wrote:
> > On Mon, 21 Mar 2022 17:48:54 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Mon, Mar 21, 2022 at 02:04:05PM +0100, Peter Zijlstra wrote:
> > > > On Mon, Mar 21, 2022 at 01:55:49PM +0100, Peter Zijlstra wrote:
> > > > > On Mon, Mar 21, 2022 at 02:03:27PM +1100, Stephen Rothwell wrote:
> > > > > > Hi all,
> > > > > > 
> > > > > > After merging the tip tree, today's linux-next build (x864 allmodconfig)
> > > > > > produced these new warnings:
> > > > > > 
> > > > > > vmlinux.o: warning: objtool: arch_rethook_prepare()+0x55: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: arch_rethook_trampoline_callback()+0x3e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x93e: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x5f2: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: unwind_next_frame()+0x4a7: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x81: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: __rethook_find_ret_addr()+0x90: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x8c: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > > vmlinux.o: warning: objtool: rethook_trampoline_handler()+0x9b: relocation to !ENDBR: arch_rethook_trampoline+0x0
> > > > > 
> > > > > Hurmph, lemme go figure out where that code comes from, I've not seen
> > > > > those.
> > > > 
> > > > Ahh, something tracing. I'll go do some patches on top of it.
> > > 
> > > The below gets rid of the objtool warnings.
> > 
> > Yes, I confirmed that.
> > 
> > > But I still think it's fairly terrible to get a (flawed) carbon copy of
> > > the kretprobe code.
> > 
> > Indeed. I would like to replace the trampoline code of kretprobe with
> > rethook, eventually. There is no reason why we keep the clone.
> > (But I need more arch maintainers help for that, there are too many
> >  archs implemented kretprobes)
> 
> FWIW, I'm more than happy to help on the arm64 side if you could Cc me for
> that; I'm aware of other things in this area I'd like to clean up for
> backtracing, too.

Thank you for your warm help. OK, let me update and submit the rethook
for arm64 :-)

Thanks.

-- 
Masami Hiramatsu <mhiramat@kernel.org>