Re: [PATCH v2] libattr: Set symbol versions for legacy syscalls via attribute or asm

[Andreas Grünbacher]

Am Do., 22. Dez. 2022 um 05:22 Uhr schrieb Alexander Miller
<alex.miller@gmx.de>:
> Currently, a linker script is used to define versioned symbols
> for the legacy linux syscalls with simple assignments.
>
> That isn't mentioned in ld's documentation as a valid method to
> set symbol versions, and while one might assume that it should work
> and the linker script is accepted, the result often isn't correct:
> gold and lld always create broken binaries, and even the bfd linker
> can create unusable symbols if used with --gc-sections or LTO.
>
> For example, the result can be a library where the function has been
> discarded and the versioned symbol is unusable, i.e.,
>     23: 00000000     0 NOTYPE  GLOBAL DEFAULT  ABS getxattr@ATTR_1.0
> instead of
>     23: 000033c0     0 FUNC    GLOBAL DEFAULT   11 getxattr@ATTR_1.0
> Meanwhile, lld doubles the version suffix: getxattr@ATTR_1.0@ATTR_1.0
>
> Not that these issues may be viewed as linker bugs, and there may
> or may not be some related improvements to linker script handling
> since I tested this a few months ago (binutils-2.37, lld-14).
> But in either case a more robust solution would be preferable.
>
> So remove the linker script entirely and set symbol versions with
> __attribute__((__symver__(...))) if available (i.e., in gcc >= 10,
> but not in clang).  Otherwise use the traditional global asm solution
> with a .symver directive.  These are the well documented methods (along
> with version scripts, which don't apply here) to set symbol versions
> and work correctly with all linkers, including older versions.
>
> A workaround that is needed for gcc < 10 not to break LTO partitioning
> with global asm is also included (__attribute__((__no_reorder__))),
> but some very old versions may still need -flto-partition=none to build
> correctly with LTO (not to going bother with completely obsolete versions).
>
> Signed-off-by: Alexander Miller <alex.miller@gmx.de>
> ---
> Changes since v1:
>  * rebased on current master
>  * add missing underscore in noreorder
>  * use double underscore variants for attribute names
>  * tried to improve title and commit message
>
>  libattr/Makemodule.am | 10 ----------
>  libattr/libattr.lds   | 12 ------------
>  libattr/syscalls.c    | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 22 deletions(-)
>  delete mode 100644 libattr/libattr.lds
>
> diff --git a/libattr/Makemodule.am b/libattr/Makemodule.am
> index 6a086e3..545da7c 100644
> --- a/libattr/Makemodule.am
> +++ b/libattr/Makemodule.am
> @@ -13,10 +13,6 @@ LTVERSION = $(LT_CURRENT):$(LT_REVISION):$(LT_AGE)
>  libattr_la_LIBADD = $(LTLIBINTL)
>  libattr_la_DEPENDENCIES = \
>         exports
> -if OS_LINUX
> -libattr_la_DEPENDENCIES += \
> -       libattr/libattr.lds
> -endif
>  libattr_la_SOURCES = \
>         libattr/attr_copy_action.c \
>         libattr/attr_copy_check.c \
> @@ -32,9 +28,3 @@ libattr_la_CFLAGS = -include libattr/libattr.h
>  libattr_la_LDFLAGS = \
>         -Wl,--version-script,$(top_srcdir)/exports \
>         -version-info $(LTVERSION)
> -if OS_LINUX
> -libattr_la_LDFLAGS += \
> -       -Wl,$(top_srcdir)/libattr/libattr.lds
> -endif
> -
> -EXTRA_DIST += libattr/libattr.lds
> diff --git a/libattr/libattr.lds b/libattr/libattr.lds
> deleted file mode 100644
> index 947f15d..0000000
> --- a/libattr/libattr.lds
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -"fgetxattr@ATTR_1.0" = libattr_fgetxattr;
> -"flistxattr@ATTR_1.0" = libattr_flistxattr;
> -"fremovexattr@ATTR_1.0" = libattr_fremovexattr;
> -"fsetxattr@ATTR_1.0" = libattr_fsetxattr;
> -"getxattr@ATTR_1.0" = libattr_getxattr;
> -"lgetxattr@ATTR_1.0" = libattr_lgetxattr;
> -"listxattr@ATTR_1.0" = libattr_listxattr;
> -"llistxattr@ATTR_1.0" = libattr_llistxattr;
> -"lremovexattr@ATTR_1.0" = libattr_lremovexattr;
> -"lsetxattr@ATTR_1.0" = libattr_lsetxattr;
> -"removexattr@ATTR_1.0" = libattr_removexattr;
> -"setxattr@ATTR_1.0" = libattr_setxattr;
> diff --git a/libattr/syscalls.c b/libattr/syscalls.c
> index 721ad7f..907560a 100644
> --- a/libattr/syscalls.c
> +++ b/libattr/syscalls.c
> @@ -26,6 +26,27 @@
>  #include <sys/syscall.h>
>  #include <sys/xattr.h>
>
> +/*
> + * Versioning of compat symbols:
> + * prefer symver attribute if available (since gcc 10),
> + * fall back to traditional .symver asm directive otherwise.
> + */
> +#ifdef __has_attribute
> +# if __has_attribute(__symver__)
> +#  define SYMVER(cn, vn) __typeof(cn) cn __attribute__((__symver__(vn)))
> +# elif __has_attribute(__no_reorder__)
> +   /*
> +    * Avoid wrong partitioning with older gcc and LTO. May not work reliably
> +    * with all versions; use -flto-partition=none if you encounter problems.
> +    */
> +#  define SYMVER(cn, vn) __typeof(cn) cn __attribute__((__no_reorder__)); \
> +                        __asm__(".symver " #cn "," vn)
> +# endif
> +#endif
> +#ifndef SYMVER
> +#  define SYMVER(cn, vn) __asm__(".symver " #cn "," vn)
> +#endif
> +
>  #ifdef HAVE_VISIBILITY_ATTRIBUTE
>  # pragma GCC visibility push(default)
>  #endif
> @@ -35,66 +56,78 @@ int libattr_setxattr(const char *path, const char *name,
>  {
>         return syscall(__NR_setxattr, path, name, value, size, flags);
>  }
> +SYMVER(libattr_setxattr, "setxattr@ATTR_1.0");
>
>  int libattr_lsetxattr(const char *path, const char *name,
>                       void *value, size_t size, int flags)
>  {
>         return syscall(__NR_lsetxattr, path, name, value, size, flags);
>  }
> +SYMVER(libattr_lsetxattr, "lsetxattr@ATTR_1.0");
>
>  int libattr_fsetxattr(int filedes, const char *name,
>                       void *value, size_t size, int flags)
>  {
>         return syscall(__NR_fsetxattr, filedes, name, value, size, flags);
>  }
> +SYMVER(libattr_fsetxattr, "fsetxattr@ATTR_1.0");
>
>  ssize_t libattr_getxattr(const char *path, const char *name,
>                          void *value, size_t size)
>  {
>         return syscall(__NR_getxattr, path, name, value, size);
>  }
> +SYMVER(libattr_getxattr, "getxattr@ATTR_1.0");
>
>  ssize_t libattr_lgetxattr(const char *path, const char *name,
>                           void *value, size_t size)
>  {
>         return syscall(__NR_lgetxattr, path, name, value, size);
>  }
> +SYMVER(libattr_lgetxattr, "lgetxattr@ATTR_1.0");
>
>  ssize_t libattr_fgetxattr(int filedes, const char *name,
>                           void *value, size_t size)
>  {
>         return syscall(__NR_fgetxattr, filedes, name, value, size);
>  }
> +SYMVER(libattr_fgetxattr, "fgetxattr@ATTR_1.0");
>
>  ssize_t libattr_listxattr(const char *path, char *list, size_t size)
>  {
>         return syscall(__NR_listxattr, path, list, size);
>  }
> +SYMVER(libattr_listxattr, "listxattr@ATTR_1.0");
>
>  ssize_t libattr_llistxattr(const char *path, char *list, size_t size)
>  {
>         return syscall(__NR_llistxattr, path, list, size);
>  }
> +SYMVER(libattr_llistxattr, "llistxattr@ATTR_1.0");
>
>  ssize_t libattr_flistxattr(int filedes, char *list, size_t size)
>  {
>         return syscall(__NR_flistxattr, filedes, list, size);
>  }
> +SYMVER(libattr_flistxattr, "flistxattr@ATTR_1.0");
>
>  int libattr_removexattr(const char *path, const char *name)
>  {
>         return syscall(__NR_removexattr, path, name);
>  }
> +SYMVER(libattr_removexattr, "removexattr@ATTR_1.0");
>
>  int libattr_lremovexattr(const char *path, const char *name)
>  {
>         return syscall(__NR_lremovexattr, path, name);
>  }
> +SYMVER(libattr_lremovexattr, "lremovexattr@ATTR_1.0");
>
>  int libattr_fremovexattr(int filedes, const char *name)
>  {
>         return syscall(__NR_fremovexattr, filedes, name);
>  }
> +SYMVER(libattr_fremovexattr, "fremovexattr@ATTR_1.0");
>
>  #ifdef HAVE_VISIBILITY_ATTRIBUTE
>  # pragma GCC visibility pop
> --
> 2.38.2

Pushed, thank you.

Andreas