[PATCH i-g-t] lib: Inline igt_x86_features() into ifunc resolvers

[Zbigniew Kempczyński]

On Thu, Mar 21, 2024 at 11:02:16AM -0400, Matt Turner wrote:
> On Thu, Mar 21, 2024 at 3:03 AM Zbigniew Kempczyński
> <zbigniew.kempczynski at intel.com> wrote:
> >
> > On Wed, Mar 20, 2024 at 11:09:11PM -0400, Matt Turner wrote:
> > > On Wed, Mar 13, 2024 at 5:09 AM Zbigniew Kempczyński
> > > <zbigniew.kempczynski at intel.com> wrote:
> > > >
> > > > On Mon, Mar 04, 2024 at 05:16:40PM -0500, Matt Turner wrote:
> > > > > Quoting https://sourceware.org/glibc/wiki/GNU_IFUNC
> > > > >
> > > > > > When LD_BIND_NOW=1 or -Wl,z,now is in effect symbols must be
> > > > > > immediately resolved at startup. In cases where an external function
> > > > > > call depends needs to be made that may fail if such a call has not
> > > > > > been initialized yet (PLT-based relocation which is processed later).
> > > > > > For example calling strlen in an IFUNC resolver built with -Wl,z,now
> > > > > > may lead to a segfault because the PLT is not yet resolved.
> > > > >
> > > > > We cannot rely on function calls through the PLT in ifunc resolvers as
> > > > > the PLT may not have been initialized yet.
> > > > >
> > > > > In practice, this causes crashes when igt is linked with -Wl,-z,now or
> > > > > when linked with the mold linker.
> > > > >
> > > > > To avoid this problem, we do two things:
> > > > >     1. move igt_x86_features() to igt_x86.h so its definition is
> > > > >        available to compilation units that call the function.
> > > > >     2. mark the ifunc resolvers with __attribute__((flatten)) to ensure
> > > > >        igt_x86_features() is inlined.
> > > > >
> > > > > Bug: https://bugs.gentoo.org/788625
> > > > > Bug: https://bugs.gentoo.org/925348
> > > > > Signed-off-by: Matt Turner <mattst88 at gmail.com>
> > > >
> > > > Hi.
> > > >
> > > > I started review of your code, but this this touches some linking intrisics
> > > > I'm not familiar with yet so I need more time to explore this. I hope
> > > > this is not a problem for you.
> > >
> > > Do you have any questions I can answer? I tried my best to explain the
> > > how and the why of the change in the commit message.
> >
> > I wanted to prepare better for the discussion - I extracted few
> > functions on which I'm reproducing segv issue when LD_BIND_NOW=1 is set.
> > I confirm, GOT doesn't contain valid function addresses so jump goes
> > to nowhere. I'm pretty sure intel_vbd_decode with LD_BIND_NOW=1 only
> > reveals the problem, but it lays within linker logic:
> >
> > # LD_BIND_NOW=1 LD_PRELOAD=./build/lib/libigt.so.0 LD_DEBUG=all ps
> >
> > ps definitely doesn't call igt_half_to_float() which in turn calls
> > unresolved igt_x86_features at plt. It looks linker first tries to
> > call all ifunc's to establish addresses, unfortunately it is doing
> > this before filling GOT. Backtrace shows we're still in _start
> > from /lib64/ld-linux-x86-64.so.2 so we even didn't get a chance
> > to get to the main().
> 
> Right, exactly. I believe ifuncs are resolved regardless of whether
> they're used.
> 
> > Before I'll give you my ack (I still want to experiment with this
> > one more day) - may you explain what for you need to bind relocs
> > immediately? This is slower than lazy binding as it resolves all
> > PLTs instead resolving only required symbols.
> 
> Gentoo/Hardened uses `-z now` to improve security. By binding upfront,
> the loader can mark the GOT as read-only for a security enhancement.
> See https://wiki.gentoo.org/wiki/Hardened/Toolchain for more details.
> 
> Apparently (according to that page) `-z now` will become the default
> in standard Gentoo profiles very soon.

I'm not going to block this change. igt_x86_features() is used only in
few places so there's not a problem to flattenize it. So from me:

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>

--
Zbigniew