Re: [PATCH 2/4] S390: Use own tbegin macro instead of __builtin_tbegin.

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2016-12-06 at 14:51 +0100, Stefan Liebler wrote:
> We have to use an own inline assembly instead of __builtin_tbegin,
> as tbegin has to filter program interruptions which can't be done with
> the builtin.  Before this change, e.g. a segmentation fault within a
> transaction, leads to a coredump where the instruction pointer points
> behind the tbegin instruction instead of real failing one.
> Now the transaction aborts and the code should be reexecuted by the
> fallback path without transactions.  The segmentation fault will
> produce a coredump with the real failing instruction.

I'm not sure that this is always a good thing to do.  It is nicer for
programmers if they can get an instruction pointer that points to where
the faulty access happened.  However:

(1) This can mask failures present in an application, because the
fallback path is not guaranteed to operate on the same data.  It may
never run into this problem.  Can inconsistencies arise due to that?
For example, is a masked segfault still visible through performance
counters or something like that?

(2) This introduces a facility to probe memory for being accessible or
not, considering that you say it masks segfaults.  It seems that this
probing may not be visible to the same extent as possible if a signal
handler were installed.  Is this relevant from a security perspective?
It may not be given that perhaps the user can do that anyway through
direct usage of transactions, or perhaps because the user would have to
be able to check whether the program is currently executing in a
transaction or not.
It might be good to at least mention this in a comment in the code.

> +++ b/sysdeps/unix/sysv/linux/s390/htm.h
> @@ -0,0 +1,149 @@
> +/* Shared HTM header.  Work around false transactional execution facility
> +   intrinsics.
> +
> +   Copyright (C) 2016 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef _HTM_H
> +#define _HTM_H 1
> +
> +#include <htmintrin.h>
> +
> +#ifdef __s390x__
> +# define TX_FPRS_BYTES 64
> +# define TX_SAVE_FPRS						\
> +  "   std %%f8, 0(%[R_FPRS])\n\t"				\
> +  "   std %%f9, 8(%[R_FPRS])\n\t"				\
> +  "   std %%f10, 16(%[R_FPRS])\n\t"				\
> +  "   std %%f11, 24(%[R_FPRS])\n\t"				\
> +  "   std %%f12, 32(%[R_FPRS])\n\t"				\
> +  "   std %%f13, 40(%[R_FPRS])\n\t"				\
> +  "   std %%f14, 48(%[R_FPRS])\n\t"				\
> +  "   std %%f15, 56(%[R_FPRS])\n\t"
> +
> +# define TX_RESTORE_FPRS					\
> +  "   ld %%f8, 0(%[R_FPRS])\n\t"				\
> +  "   ld %%f9, 8(%[R_FPRS])\n\t"				\
> +  "   ld %%f10, 16(%[R_FPRS])\n\t"				\
> +  "   ld %%f11, 24(%[R_FPRS])\n\t"				\
> +  "   ld %%f12, 32(%[R_FPRS])\n\t"				\
> +  "   ld %%f13, 40(%[R_FPRS])\n\t"				\
> +  "   ld %%f14, 48(%[R_FPRS])\n\t"				\
> +  "   ld %%f15, 56(%[R_FPRS])\n\t"
> +
> +#else
> +
> +# define TX_FPRS_BYTES 16
> +# define TX_SAVE_FPRS						\
> +  "   std %%f4, 0(%[R_FPRS])\n\t"				\
> +  "   std %%f6, 8(%[R_FPRS])\n\t"
> +
> +# define TX_RESTORE_FPRS					\
> +  "   ld %%f4, 0(%[R_FPRS])\n\t"				\
> +  "   ld %%f6, 8(%[R_FPRS])\n\t"
> +
> +#endif /* ! __s390x__  */
> +
> +/* Use own inline assembly instead of __builtin_tbegin, as tbegin
> +   has to filter program interruptions which can't be done with the builtin.
> +   Now the fprs have to be saved / restored here, too.
> +   The fpc is also not saved / restored with the builtin.
> +   The used inline assembly does not clobber the volatile fprs / vrs!
> +   Clobbering the latter ones would force the compiler to save / restore
> +   the call saved fprs as those overlap with the vrs, but they only need to be
> +   restored if the transaction fails but not if the transaction is successfully
> +   started.  Thus the user of the tbegin macros in this header file has to
> +   compile the file / function with -msoft-float.  It prevents gcc from using
> +   fprs / vrs.  */
> +#define __libc_tbegin(tdb)						\
> +  ({ int __ret;								\
> +     int __fpc;								\
> +     char __fprs[TX_FPRS_BYTES];					\
> +     __asm__ __volatile__ (".machine push\n\t"				\
> +			   ".machinemode \"zarch_nohighgprs\"\n\t"	\
> +			   ".machine \"all\"\n\t"			\
> +			   /* Save state at the outermost transaction.	\
> +			      As extracting nesting depth is expensive	\
> +			      on at least zEC12, save fprs at inner	\
> +			      transactions, too.			\
> +			      The fpc and fprs are saved here as they	\
> +			      are not saved by tbegin.  There exist no	\
> +			      call-saved vrs, thus they are not saved	\
> +			      here.  */					\
> +			   "   efpc %[R_FPC]\n\t"			\
> +			   TX_SAVE_FPRS					\
> +			   /* Begin transaction: save all gprs, allow	\
> +			      ar modification and fp operations.  Some	\
> +			      program-interruptions (e.g. a null	\
> +			      pointer access) are filtered and the	\
> +			      trancsaction will abort.  In this case	\
> +			      the normal lock path will execute it	\
> +			      again and result in a core dump wich does	\
> +			      now show at tbegin but the real executed	\
> +			      instruction.  */				\
> +			   "   tbegin 0, 0xFF0E\n\t"			\
> +			   /* Branch away in abort case (this is the	\
> +			      prefered sequence.  See PoP in chapter 5	\
> +			      Transactional-Execution Facility		\
> +			      Operation).  */				\
> +			   "   jnz 0f\n\t"				\
> +			   /* Transaction has successfully started.  */	\
> +			   "   lhi %[R_RET], 0\n\t"			\
> +			   "   j 1f\n\t"				\
> +			   /* Transaction has aborted.  Now we are at	\
> +			      the outermost transaction.  Restore fprs	\
> +			      and fpc. */				\
> +			   "0: ipm %[R_RET]\n\t"			\
> +			   "   srl %[R_RET], 28\n\t"			\
> +			   "   sfpc %[R_FPC]\n\t"			\
> +			   TX_RESTORE_FPRS				\
> +			   "1:\n\t"					\
> +			   ".machine pop\n"				\
> +			   : [R_RET] "=&d" (__ret),			\
> +			     [R_FPC] "=&d" (__fpc)			\
> +			   : [R_FPRS] "a" (__fprs)			\
> +			   : "cc", "memory");				\
> +     __ret;								\
> +     })
> +
> +/* These builtins are correct.  Use them.  */

Is "correct" the right word here?  I suppose they are always correct,
it's just that you want something different for glibc internally.