Patchwork x86/kernel: Fix more -Wmissing-prototypes warnings

login
register
mail settings
Submitter Borislav Petkov
Date Dec. 5, 2018, 5:15 p.m.
Message ID <20181205171547.GM29510@zn.tnic>
Download mbox | patch
Permalink /patch/673365/
State New
Headers show

Comments

Borislav Petkov - Dec. 5, 2018, 5:15 p.m.
On Wed, Dec 05, 2018 at 05:25:26PM +0100, Ingo Molnar wrote:
> Also, preferably the prototype should be eliminated via proper ordering 
> of functions from lower level to higher levels.

Well, that trampoline_handler() is special as we call it from inline asm
and I see Masami has marked it __visible etc.

So doing this below still builds and the trampoline_handler doesn't get
discarded but maybe there's a reason for it not being static.

Masami?

---
Masami Hiramatsu - Dec. 7, 2018, 2:42 p.m.
Hi,

On Wed, 5 Dec 2018 18:15:47 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Dec 05, 2018 at 05:25:26PM +0100, Ingo Molnar wrote:
> > Also, preferably the prototype should be eliminated via proper ordering 
> > of functions from lower level to higher levels.
> 
> Well, that trampoline_handler() is special as we call it from inline asm
> and I see Masami has marked it __visible etc.
> 
> So doing this below still builds and the trampoline_handler doesn't get
> discarded but maybe there's a reason for it not being static.
> 
> Masami?

Hmm, I just thought that the symbol only referred from inline asm should
be visible. But if it is OK for any version of supported gcc and clang,
I'm good to remove it. :-) (IOW, I just concerned about older gcc)

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> 
> ---
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 6480056d370f..308bf103cc73 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -66,8 +66,6 @@
>  
>  #include "common.h"
>  
> -void *trampoline_handler(struct pt_regs *regs);
> -
>  DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
>  DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
>  
> @@ -753,7 +751,7 @@ STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
>  /*
>   * Called from kretprobe_trampoline
>   */
> -__visible __used void *trampoline_handler(struct pt_regs *regs)
> +static __used void *trampoline_handler(struct pt_regs *regs)
>  {
>  	struct kretprobe_instance *ri = NULL;
>  	struct hlist_head *head, empty_rp;
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.
Borislav Petkov - Dec. 7, 2018, 7:48 p.m.
On Fri, Dec 07, 2018 at 11:42:10PM +0900, Masami Hiramatsu wrote:
> Hmm, I just thought that the symbol only referred from inline asm should
> be visible. But if it is OK for any version of supported gcc and clang,
> I'm good to remove it. :-) (IOW, I just concerned about older gcc)

I just tried two gcc versions: 8.2 and 4.8.5. Both asms looks good:

ffffffff81044690 <kretprobe_trampoline>:
...


ffffffff810446ab:       41 57                   push   %r15
ffffffff810446ad:       48 89 e7                mov    %rsp,%rdi
ffffffff810446b0:       e8 db 01 00 00          callq  ffffffff81044890 <trampoline_handler>

there's the CALL...

ffffffff810446b5:       48 89 84 24 98 00 00    mov    %rax,0x98(%rsp)
ffffffff810446bc:       00

... and the handler is at the expected address.

ffffffff81044890 <trampoline_handler>:
ffffffff81044890:       e8 ab c9 7b 00          callq  ffffffff81801240 <__fentry__>
ffffffff81044895:       41 57                   push   %r15
ffffffff81044897:       41 56                   push   %r14
ffffffff81044899:       41 55                   push   %r13
ffffffff8104489b:       49 89 fd                mov    %rdi,%r13

Dunno, if you feel like there might be some trouble with some compilers,
I can keep the hunk below as a separate patch and revert it when it
explodes somewhere...?

> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Thank you,
> 
> > 
> > ---
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index 6480056d370f..308bf103cc73 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -66,8 +66,6 @@
> >  
> >  #include "common.h"
> >  
> > -void *trampoline_handler(struct pt_regs *regs);
> > -
> >  DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> >  DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> >  
> > @@ -753,7 +751,7 @@ STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> >  /*
> >   * Called from kretprobe_trampoline
> >   */
> > -__visible __used void *trampoline_handler(struct pt_regs *regs)
> > +static __used void *trampoline_handler(struct pt_regs *regs)
> >  {
> >  	struct kretprobe_instance *ri = NULL;
> >  	struct hlist_head *head, empty_rp;
> > 
> > -- 
> > Regards/Gruss,
> >     Boris.
> > 
> > Good mailing practices for 400: avoid top-posting and trim the reply.
> 
> 
> -- 
> Masami Hiramatsu <mhiramat@kernel.org>
Masami Hiramatsu - Dec. 8, 2018, 5:56 a.m.
On Fri, 7 Dec 2018 20:48:47 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Dec 07, 2018 at 11:42:10PM +0900, Masami Hiramatsu wrote:
> > Hmm, I just thought that the symbol only referred from inline asm should
> > be visible. But if it is OK for any version of supported gcc and clang,
> > I'm good to remove it. :-) (IOW, I just concerned about older gcc)
> 
> I just tried two gcc versions: 8.2 and 4.8.5. Both asms looks good:
> 
> ffffffff81044690 <kretprobe_trampoline>:
> ...
> 
> 
> ffffffff810446ab:       41 57                   push   %r15
> ffffffff810446ad:       48 89 e7                mov    %rsp,%rdi
> ffffffff810446b0:       e8 db 01 00 00          callq  ffffffff81044890 <trampoline_handler>
> 
> there's the CALL...
> 
> ffffffff810446b5:       48 89 84 24 98 00 00    mov    %rax,0x98(%rsp)
> ffffffff810446bc:       00
> 
> ... and the handler is at the expected address.
> 
> ffffffff81044890 <trampoline_handler>:
> ffffffff81044890:       e8 ab c9 7b 00          callq  ffffffff81801240 <__fentry__>
> ffffffff81044895:       41 57                   push   %r15
> ffffffff81044897:       41 56                   push   %r14
> ffffffff81044899:       41 55                   push   %r13
> ffffffff8104489b:       49 89 fd                mov    %rdi,%r13
> 
> Dunno, if you feel like there might be some trouble with some compilers,
> I can keep the hunk below as a separate patch and revert it when it
> explodes somewhere...?

OK, then please remove it. :-) Even if any problem occurs, which must be
build error, so we can revert it in that case.

Thank you!

> 
> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Thank you,
> > 
> > > 
> > > ---
> > > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > > index 6480056d370f..308bf103cc73 100644
> > > --- a/arch/x86/kernel/kprobes/core.c
> > > +++ b/arch/x86/kernel/kprobes/core.c
> > > @@ -66,8 +66,6 @@
> > >  
> > >  #include "common.h"
> > >  
> > > -void *trampoline_handler(struct pt_regs *regs);
> > > -
> > >  DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> > >  DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> > >  
> > > @@ -753,7 +751,7 @@ STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> > >  /*
> > >   * Called from kretprobe_trampoline
> > >   */
> > > -__visible __used void *trampoline_handler(struct pt_regs *regs)
> > > +static __used void *trampoline_handler(struct pt_regs *regs)
> > >  {
> > >  	struct kretprobe_instance *ri = NULL;
> > >  	struct hlist_head *head, empty_rp;
> > > 
> > > -- 
> > > Regards/Gruss,
> > >     Boris.
> > > 
> > > Good mailing practices for 400: avoid top-posting and trim the reply.
> > 
> > 
> > -- 
> > Masami Hiramatsu <mhiramat@kernel.org>
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Good mailing practices for 400: avoid top-posting and trim the reply.

Patch

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 6480056d370f..308bf103cc73 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -66,8 +66,6 @@ 
 
 #include "common.h"
 
-void *trampoline_handler(struct pt_regs *regs);
-
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
@@ -753,7 +751,7 @@  STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
 /*
  * Called from kretprobe_trampoline
  */
-__visible __used void *trampoline_handler(struct pt_regs *regs)
+static __used void *trampoline_handler(struct pt_regs *regs)
 {
 	struct kretprobe_instance *ri = NULL;
 	struct hlist_head *head, empty_rp;