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

From: Nathan Chancellor <natechancellor@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: ndesaulniers@google.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] inet/connection_sock: prefer _THIS_IP_ to current_text_addr
Date: Wed, 1 Aug 2018 22:08:18 -0700	[thread overview]
Message-ID: <20180802050818.GA18742@flashbox> (raw)
In-Reply-To: <20180802045825.GA16769@flashbox>

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