acl-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] libattr: Set symbol versions for legacy syscalls via attr


From: Corinna Vinschen
Subject: Re: [PATCH v2] libattr: Set symbol versions for legacy syscalls via attribute or asm
Date: Wed, 18 Jan 2023 12:53:53 +0100

Hi guys,

is there any progress in terms of merging this patch into attr?

I was working on getting attr-2.5.1 working on Cygwin last week.

The most recent package in the distro was 2.4.48.  I had to tweak the
package a bit and I never pushed patches upstream.  To make up for
that, I worked on current attr git master and provided my patches
in https://savannah.nongnu.org/bugs/?63670.

However, only *after* doing that, I scanned the acl-devel mailing
list and found that this patch here was available.

Turns out, with Alexander's patch (which looks pretty good to me, btw)
the patch I created for Cygwin was not required at all!  I only had to
add "-no-undefined" to LDFLAGS in the distro spec file for attr, and
voilĂ , latest attr works fine on CYgwin as well.

So, if anything is holding up this patch from Alexander, I'd be glad if
my mail could vote for its inclusion into attr git master by providing
a good example for its usability :)


Thanks,
Corinna


On Dec 22 05:21, Alexander Miller wrote:
> 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
> 
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]