[PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

As part of the effort to reduce the code duplication between _THIS_IP_
and current_text_addr(), let's consolidate callers of
current_text_addr() to use _THIS_IP_.

Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/x86/include/asm/kexec.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
index f327236f0fa7..86924d594ecd 100644
--- a/arch/x86/include/asm/kexec.h
+++ b/arch/x86/include/asm/kexec.h
@@ -21,6 +21,7 @@
 #ifndef __ASSEMBLY__
 
 #include <linux/string.h>
+#include <linux/kernel.h>
 
 #include <asm/page.h>
 #include <asm/ptrace.h>
@@ -132,7 +133,7 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
 		asm volatile("movl %%cs, %%eax;" :"=a"(newregs->cs));
 		asm volatile("pushfq; popq %0" :"=m"(newregs->flags));
 #endif
-		newregs->ip = (unsigned long)current_text_addr();
+		newregs->ip = _THIS_IP_;
 	}
 }
 
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

bumping for review
On Wed, Aug 1, 2018 at 2:08 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> As part of the effort to reduce the code duplication between _THIS_IP_
> and current_text_addr(), let's consolidate callers of
> current_text_addr() to use _THIS_IP_.
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  arch/x86/include/asm/kexec.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> index f327236f0fa7..86924d594ecd 100644
> --- a/arch/x86/include/asm/kexec.h
> +++ b/arch/x86/include/asm/kexec.h
> @@ -21,6 +21,7 @@
>  #ifndef __ASSEMBLY__
>
>  #include <linux/string.h>
> +#include <linux/kernel.h>
>
>  #include <asm/page.h>
>  #include <asm/ptrace.h>
> @@ -132,7 +133,7 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
>                 asm volatile("movl %%cs, %%eax;" :"=a"(newregs->cs));
>                 asm volatile("pushfq; popq %0" :"=m"(newregs->flags));
>  #endif
> -               newregs->ip = (unsigned long)current_text_addr();
> +               newregs->ip = _THIS_IP_;
>         }
>  }
>
> --
> 2.18.0.597.ga71716f1ad-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

+ Simon (kexec)

bumping for review.
On Wed, Aug 8, 2018 at 4:06 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> bumping for review
> On Wed, Aug 1, 2018 at 2:08 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > As part of the effort to reduce the code duplication between _THIS_IP_
> > and current_text_addr(), let's consolidate callers of
> > current_text_addr() to use _THIS_IP_.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  arch/x86/include/asm/kexec.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> > index f327236f0fa7..86924d594ecd 100644
> > --- a/arch/x86/include/asm/kexec.h
> > +++ b/arch/x86/include/asm/kexec.h
> > @@ -21,6 +21,7 @@
> >  #ifndef __ASSEMBLY__
> >
> >  #include <linux/string.h>
> > +#include <linux/kernel.h>
> >
> >  #include <asm/page.h>
> >  #include <asm/ptrace.h>
> > @@ -132,7 +133,7 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
> >                 asm volatile("movl %%cs, %%eax;" :"=a"(newregs->cs));
> >                 asm volatile("pushfq; popq %0" :"=m"(newregs->flags));
> >  #endif
> > -               newregs->ip = (unsigned long)current_text_addr();
> > +               newregs->ip = _THIS_IP_;
> >         }
> >  }
> >
> > --
> > 2.18.0.597.ga71716f1ad-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

+ akpm, Linus

Bumping for review.

On Mon, Aug 13, 2018 at 2:32 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> + Simon (kexec)
>
> bumping for review.
> On Wed, Aug 8, 2018 at 4:06 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> >
> > bumping for review
> > On Wed, Aug 1, 2018 at 2:08 PM Nick Desaulniers <ndesaulniers@google.com> wrote:
> > >
> > > As part of the effort to reduce the code duplication between _THIS_IP_
> > > and current_text_addr(), let's consolidate callers of
> > > current_text_addr() to use _THIS_IP_.
> > >
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > ---
> > >  arch/x86/include/asm/kexec.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> > > index f327236f0fa7..86924d594ecd 100644
> > > --- a/arch/x86/include/asm/kexec.h
> > > +++ b/arch/x86/include/asm/kexec.h
> > > @@ -21,6 +21,7 @@
> > >  #ifndef __ASSEMBLY__
> > >
> > >  #include <linux/string.h>
> > > +#include <linux/kernel.h>
> > >
> > >  #include <asm/page.h>
> > >  #include <asm/ptrace.h>
> > > @@ -132,7 +133,7 @@ static inline void crash_setup_regs(struct pt_regs *newregs,
> > >                 asm volatile("movl %%cs, %%eax;" :"=a"(newregs->cs));
> > >                 asm volatile("pushfq; popq %0" :"=m"(newregs->flags));
> > >  #endif
> > > -               newregs->ip = (unsigned long)current_text_addr();
> > > +               newregs->ip = _THIS_IP_;
> > >         }
> > >  }
> > >
> > > --
> > > 2.18.0.597.ga71716f1ad-goog
> > >

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[Linus Torvalds]

On Mon, Aug 20, 2018 at 10:58 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> + akpm, Linus
>
> Bumping for review.

Ugh. I am not personally a huge fan of this endless "fix up one at a time".

Just do a patch that removes current_text_addr() entirely and be done
with it, if that's what we want the end result to be.

Don't bother with these small "let's remove the remaining ones one by
one". Just get it over and done with.

                Linus

Re: [PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[Eric W. Biederman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, Aug 20, 2018 at 10:58 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>
>> + akpm, Linus
>>
>> Bumping for review.
>
> Ugh. I am not personally a huge fan of this endless "fix up one at a time".
>
> Just do a patch that removes current_text_addr() entirely and be done
> with it, if that's what we want the end result to be.
>
> Don't bother with these small "let's remove the remaining ones one by
> one". Just get it over and done with.

One is generic code the other is assembly but the both appear to do the
same thing without unexpected complexity.

That said the patch earlier in this thread has clearly never been
compiled as it is using THIS_IP instead of _THIS_IP_ and there is not
a define of THIS_IP in the kernel.

Eric

Re: [PATCH] x86/kexec: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

On Tue, Aug 21, 2018 at 5:40 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Mon, Aug 20, 2018 at 10:58 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> >>
> >> + akpm, Linus
> >>
> >> Bumping for review.
> >
> > Ugh. I am not personally a huge fan of this endless "fix up one at a time".
> >
> > Just do a patch that removes current_text_addr() entirely and be done
> > with it, if that's what we want the end result to be.
> >
> > Don't bother with these small "let's remove the remaining ones one by
> > one". Just get it over and done with.

Linus,
No problem. Will send a V2 that rips off the band-aid.

I explained my thoughts on the process more in
https://lkml.org/lkml/2018/8/1/1689, which akpm agreed with.  I was
hoping that I wouldn't have to bother you with a tree-wide change, but
if code review takes 3 weeks...

Also, I was hoping to save the small but many clean ups as starter
patches for some newbies that we're asking me how to get started
contributing to the kernel, but I can find other good first bugs for
them.

>
> One is generic code the other is assembly but the both appear to do the
> same thing without unexpected complexity.
>
> That said the patch earlier in this thread has clearly never been
> compiled as it is using THIS_IP instead of _THIS_IP_ and there is not
> a define of THIS_IP in the kernel.

Eric,
Please triple check your email client or whatever you're using to
download patches, as I suspect one of them may be stripping
leading+trailing underscores.  That would be quite problematic for
code review or applying patch files.  I've seen some text renderers
treat the underscores as markdown and italicize the contained word.
https://lkml.org/lkml/2018/8/1/1608 shows the underscores.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] treewide: remove current_text_addr

[Nicholas Piggin]

[ Trimmed the cc list because my SMTP didn't accept that many
addresses. ]

On Sun, 26 Aug 2018 13:25:14 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sun, Aug 26, 2018 at 12:32 PM H. Peter Anvin <hpa@zytor.com> wrote:
> >
> > Here is a full-blown (user space) test program demonstrating the whole
> > technique and how to use it.  
> 
> So while I agree that some _THIS_IP_ users might be better off being
> converted to __builtin_return_address(0) at the caller, I also think
> that the whole "notailcall" thing shows why that can easily be more
> problematic than just our currnet _THIS_IP_ solution.
> 
> Honestly, I'd suggest:
> 
>  - just do the current_text_addr() to _THIS_IP_ conversion
> 
>  - keep _THIS_IP_ and make it be the generic one, and screw the whole
> "some architectures might implement is better" issue. Nobody cares.
> 
>  - try to convince people to move away from the "we want the kernel
> instruction pointer for the call" model entirely, and consider this a
> "legacy" issue.
> 
> The whole instruction pointer is a nasty thing. We should discourage
> it and not make complex infrastructure for it.
> 
> Instead, maybe we could encourage something like
> 
>   struct kernel_loc { const char *file; const char *fn; int line; };
> 
>   #define __GEN_LOC__(n) \
>         ({ static const struct kernel_loc n = { \
>                 __FILE__, __FUNCTION__, __LINE__  \
>            }; &n; })
> 
>   #define _THIS_LOC_ __GEN_LOC__(__UNIQUE_ID(loc))
> 
> which is a hell of a lot nicer to use, and actually allows gcc to
> optimize things (try it: if you pass a _THIS_LOC_ off to an inline
> function, and that inline function uses the name and line number, gcc
> will pick them up directly, without the extra structure dereference.
> 
> Wouldn't it be much nicer to pass these kinds of "location pointer"
> around, rather than the nasty _THIS_IP_ thing?

Seems nice. Do you even need this unique ID thing? AFAIKS the name
would never really be useful.

It could perhaps go into a cold data section too, I assume the common
case is that you do not access it. Although gcc will end up putting
the file and function names into regular rodata.

Possibly we could add a printk specifier for it, pass it through to
existing BUG, etc macros that want exactly this, etc. Makes a lot of
sense.

Thanks,
Nick