[PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[PATCH] inet/connection_sock: 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>
---
 include/net/inet_connection_sock.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
index 0a6c9e0f2b5a..f2a87e85c07b 100644
--- a/include/net/inet_connection_sock.h
+++ b/include/net/inet_connection_sock.h
@@ -19,6 +19,7 @@
 #include <linux/string.h>
 #include <linux/timer.h>
 #include <linux/poll.h>
+#include <linux/kernel.h>
 
 #include <net/inet_sock.h>
 #include <net/request_sock.h>
@@ -224,7 +225,7 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what,
 
 	if (when > max_when) {
 		pr_debug("reset_xmit_timer: sk=%p %d when=0x%lx, caller=%p\n",
-			 sk, what, when, current_text_addr());
+			 sk, what, when, (void *)_THIS_IP_);
 		when = max_when;
 	}
 
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[David Miller]

From: Nick Desaulniers <ndesaulniers@google.com>
Date: Wed,  1 Aug 2018 14:57:59 -0700

> -			 sk, what, when, current_text_addr());
> +			 sk, what, when, (void *)_THIS_IP_);

Is a fugly macro in all caps really better than a function()?

I'm really surprised that _THIS_IP_ is being chosen over
current_text_addr(), seriously.

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[Nathan Chancellor]

On Wed, Aug 01, 2018 at 06:42:08PM -0700, David Miller wrote:
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Wed,  1 Aug 2018 14:57:59 -0700
> 
> > -			 sk, what, when, current_text_addr());
> > +			 sk, what, when, (void *)_THIS_IP_);
> 
> Is a fugly macro in all caps really better than a function()?
> 
> I'm really surprised that _THIS_IP_ is being chosen over
> current_text_addr(), seriously.

Hi Dave,

current_text_addr is only used in five places in the entire kernel and
this is the only non-architecture specific use.

include/net/inet_connection_sock.h:227:			 sk, what, when, current_text_addr());
arch/sh/include/asm/kexec.h:64:		newregs->pc = (unsigned long)current_text_addr();
arch/sh/kernel/dwarf.c:602:		pc = (unsigned long)current_text_addr();
arch/x86/include/asm/kexec.h:135:		newregs->ip = (unsigned long)current_text_addr();
arch/parisc/kernel/unwind.c:442:	r.iaoq[0] = (unsigned long) current_text_addr();

We're trying to merge it with _THIS_IP_ before for most architectures,
it is defined nearly identically to _THIS_IP_:

#define _THIS_IP_  ({ __label__ __here; __here: (unsigned long)&&__here; })

#define current_text_addr() ({ __label__ _l; _l: &&_l; })

* arc
* arm
* arm64
* h8300
* hexagon
* m68k
* mips
* nds32
* nios2
* openrisc
* powerpc
* riscv
* unicore32
* xtensa

Ultimately, we're trying to turn off a new Clang warning in _THIS_IP_
which requires some changes to work around what is believed to be a GCC
bug (see https://lore.kernel.org/patchwork/patch/969101/).

I guess the alternative to this patch is to just define current_text_addr
as _THIS_IP_ for all of those architectures, I am not sure how that
filters into Nick's plan (I think the goal of this was to try and avoid
getting all of the architecture folks involved).

Thanks for your time!
Nathan

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[Nathan Chancellor]

On Wed, Aug 01, 2018 at 09:58:25PM -0700, Nathan Chancellor wrote:
> On Wed, Aug 01, 2018 at 06:42:08PM -0700, David Miller wrote:
> > From: Nick Desaulniers <ndesaulniers@google.com>
> > Date: Wed,  1 Aug 2018 14:57:59 -0700
> > 
> > > -			 sk, what, when, current_text_addr());
> > > +			 sk, what, when, (void *)_THIS_IP_);
> > 
> > Is a fugly macro in all caps really better than a function()?
> > 
> > I'm really surprised that _THIS_IP_ is being chosen over
> > current_text_addr(), seriously.
> 
> Hi Dave,

Sorry for the second email so quick after the first, needed to clarify
two things.

> 
> current_text_addr is only used in five places in the entire kernel and
> this is the only non-architecture specific use.
> 
> include/net/inet_connection_sock.h:227:			 sk, what, when, current_text_addr());
> arch/sh/include/asm/kexec.h:64:		newregs->pc = (unsigned long)current_text_addr();
> arch/sh/kernel/dwarf.c:602:		pc = (unsigned long)current_text_addr();
> arch/x86/include/asm/kexec.h:135:		newregs->ip = (unsigned long)current_text_addr();
> arch/parisc/kernel/unwind.c:442:	r.iaoq[0] = (unsigned long) current_text_addr();
> 
> We're trying to merge it with _THIS_IP_ before for most architectures,

because for most...

> it is defined nearly identically to _THIS_IP_:
> 
> #define _THIS_IP_  ({ __label__ __here; __here: (unsigned long)&&__here; })
> 
> #define current_text_addr() ({ __label__ _l; _l: &&_l; })
> 
> * arc
> * arm
> * arm64
> * h8300
> * hexagon
> * m68k
> * mips
> * nds32
> * nios2
> * openrisc
> * powerpc
> * riscv
> * unicore32
> * xtensa
> 
> Ultimately, we're trying to turn off a new Clang warning in _THIS_IP_
> which requires some changes to work around what is believed to be a GCC
> bug (see https://lore.kernel.org/patchwork/patch/969101/).

and I should have mentioned that current_text_addr suffers from the
exact same issue so we're trying not to duplicate it 14 times across
the kernel.

> I guess the alternative to this patch is to just define current_text_addr
> as _THIS_IP_ for all of those architectures, I am not sure how that
> filters into Nick's plan (I think the goal of this was to try and avoid
> getting all of the architecture folks involved).
> 
> Thanks for your time!
> Nathan

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

On Wed, Aug 1, 2018 at 6:42 PM David Miller <davem@davemloft.net> wrote:
>
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Wed,  1 Aug 2018 14:57:59 -0700
>
> > -                      sk, what, when, current_text_addr());
> > +                      sk, what, when, (void *)_THIS_IP_);
>
> Is a fugly macro in all caps really better than a function()?
>
> I'm really surprised that _THIS_IP_ is being chosen over
> current_text_addr(), seriously.

See: https://lkml.org/lkml/2018/8/1/1689

I'm happy to rename it on your suggestion, after we've consolidated them.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[David Miller]

From: Nick Desaulniers <ndesaulniers@google.com>
Date: Wed,  1 Aug 2018 14:57:59 -0700

> 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>

I'll ACk this for now:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote:
>
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Wed,  1 Aug 2018 14:57:59 -0700
>
> > 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>
>
> I'll ACk this for now:
>
> Acked-by: David S. Miller <davem@davemloft.net>

Thank you David.  Should you take it into the net tree, can you please add:

Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4

to the commit message? This will help us as Nathan stated previously
in this thread.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[David Miller]

From: Nick Desaulniers <ndesaulniers@google.com>
Date: Thu, 2 Aug 2018 15:10:00 -0700

> On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote:
>>
>> From: Nick Desaulniers <ndesaulniers@google.com>
>> Date: Wed,  1 Aug 2018 14:57:59 -0700
>>
>> > 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>
>>
>> I'll ACk this for now:
>>
>> Acked-by: David S. Miller <davem@davemloft.net>
> 
> Thank you David.  Should you take it into the net tree, can you please add:
> 
> Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4
> 
> to the commit message? This will help us as Nathan stated previously
> in this thread.

Why in the world is this a stable candidate?  It doesn't fix a bug.

My understanding is that you are trying to do a consolidation of code
and clean things up so that only one interface is used.

That means it should, at best, go to the net-next tree.

Thank you.

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

On Thu, Aug 2, 2018 at 3:17 PM David Miller <davem@davemloft.net> wrote:
>
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Thu, 2 Aug 2018 15:10:00 -0700
>
> > On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote:
> >>
> >> From: Nick Desaulniers <ndesaulniers@google.com>
> >> Date: Wed,  1 Aug 2018 14:57:59 -0700
> >>
> >> > 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>
> >>
> >> I'll ACk this for now:
> >>
> >> Acked-by: David S. Miller <davem@davemloft.net>
> >
> > Thank you David.  Should you take it into the net tree, can you please add:
> >
> > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4
> >
> > to the commit message? This will help us as Nathan stated previously
> > in this thread.
>
> Why in the world is this a stable candidate?  It doesn't fix a bug.
>
> My understanding is that you are trying to do a consolidation of code
> and clean things up so that only one interface is used.
>
> That means it should, at best, go to the net-next tree.
>
> Thank you.

+ gkh

We're trying to provide backports for all clang related bugs and
warnings to the stable trees back through 4.4.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[Nick Desaulniers]

On Thu, Aug 2, 2018 at 3:17 PM David Miller <davem@davemloft.net> wrote:
>
> From: Nick Desaulniers <ndesaulniers@google.com>
> Date: Thu, 2 Aug 2018 15:10:00 -0700
>
> > On Thu, Aug 2, 2018 at 2:42 PM David Miller <davem@davemloft.net> wrote:
> >>
> >> From: Nick Desaulniers <ndesaulniers@google.com>
> >> Date: Wed,  1 Aug 2018 14:57:59 -0700
> >>
> >> > 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>
> >>
> >> I'll ACk this for now:
> >>
> >> Acked-by: David S. Miller <davem@davemloft.net>
> >
> > Thank you David.  Should you take it into the net tree, can you please add:
> >
> > Cc: stable@vger.kernel.org # 4.17, 4.14, 4.9, 4.4
> >
> > to the commit message? This will help us as Nathan stated previously
> > in this thread.
>
> Why in the world is this a stable candidate?  It doesn't fix a bug.
>
> My understanding is that you are trying to do a consolidation of code
> and clean things up so that only one interface is used.
>
> That means it should, at best, go to the net-next tree.

David,
Can you please pick this up for the net-next tree then?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr

[David Miller]

From: Nick Desaulniers <ndesaulniers@google.com>
Date: Mon, 13 Aug 2018 14:10:08 -0700

> Can you please pick this up for the net-next tree then?

Sure, done.