[v3,1/2] package/lvm2: Fix runtime crash when using uclibc

Message ID 20180726171524.479-1-m.niestroj@grinn-global.com
State Accepted
Headers show
Series
  • [v3,1/2] package/lvm2: Fix runtime crash when using uclibc
Related show

Commit Message

Marcin Niestroj July 26, 2018, 5:15 p.m.
When using uclibc libdevmapper.so was calling dm_task_get_info_base()
function recursively, leading to segmentation fault. This was
happening because uclibc linker loader just takes first existing
'dm_task_get_info' (which is 'dm_task_get_info_base') symbol in elf
binary, instead of default version.

Add upstreamable lvm2 patch [1], which introduces
--enable-symvers[=STYLE] switch. Use that switch to disable symbol
versions, as we do not plan to support binaries compiled against
old libdevmapper library.

[1] https://www.redhat.com/archives/dm-devel/2018-July/msg00187.html

Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
---
Changes v2 -> v3: none

Changes v1 -> v2:
 * Add SoB in patch itself (suggested by Thomas)
 * Make this patch first in series, so it can be applied for LTS
   branches (sugested by Thomas)
 * Rebase patch on current lvm2 Buildroot master version (2.02.173),
   so it can apply cleanly

 ...gure-Introduce-enable-symvers-option.patch | 277 ++++++++++++++++++
 package/lvm2/lvm2.mk                          |   3 +-
 2 files changed, 279 insertions(+), 1 deletion(-)
 create mode 100644 package/lvm2/0001-configure-Introduce-enable-symvers-option.patch

Comments

Thomas Petazzoni Aug. 14, 2018, 8:47 p.m. | #1
Hello Marcin,

Once again, thanks for this work!

On Thu, 26 Jul 2018 19:15:23 +0200, Marcin Niestroj wrote:
> When using uclibc libdevmapper.so was calling dm_task_get_info_base()
> function recursively, leading to segmentation fault. This was
> happening because uclibc linker loader just takes first existing
> 'dm_task_get_info' (which is 'dm_task_get_info_base') symbol in elf
> binary, instead of default version.
> 
> Add upstreamable lvm2 patch [1], which introduces
> --enable-symvers[=STYLE] switch. Use that switch to disable symbol
> versions, as we do not plan to support binaries compiled against
> old libdevmapper library.
> 
> [1] https://www.redhat.com/archives/dm-devel/2018-July/msg00187.html
> 
> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
> ---
> Changes v2 -> v3: none

I have added a reference to the Buildroot bug report being fixed by
this, and I've applied to master.

Two questions:

 (1) Could you try to push this forward in terms of upstreaming
     acceptance ? I know you have submitted the patch, but I looked
     today and apparently you haven't received any feedback.

 (2) It would be nicer if the availability of symbol versioning could
     be auto-detected. Is there a compile-time test that can be done to
     verify if symbol versioning is available ? This would perhaps help
     make the patch even more acceptable upstream.

Thanks!

Thomas
Peter Korsgaard Aug. 24, 2018, 8:34 a.m. | #2
>>>>> "Marcin" == Marcin Niestroj <m.niestroj@grinn-global.com> writes:

 > When using uclibc libdevmapper.so was calling dm_task_get_info_base()
 > function recursively, leading to segmentation fault. This was
 > happening because uclibc linker loader just takes first existing
 > 'dm_task_get_info' (which is 'dm_task_get_info_base') symbol in elf
 > binary, instead of default version.

 > Add upstreamable lvm2 patch [1], which introduces
 > --enable-symvers[=STYLE] switch. Use that switch to disable symbol
 > versions, as we do not plan to support binaries compiled against
 > old libdevmapper library.

 > [1] https://www.redhat.com/archives/dm-devel/2018-July/msg00187.html

 > Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
 > ---
 > Changes v2 -> v3: none

 > Changes v1 -> v2:
 >  * Add SoB in patch itself (suggested by Thomas)
 >  * Make this patch first in series, so it can be applied for LTS
 >    branches (sugested by Thomas)
 >  * Rebase patch on current lvm2 Buildroot master version (2.02.173),
 >    so it can apply cleanly

Committed to 2018.02.x and 2018.05.x, thanks.
Marcin Niestroj Aug. 31, 2018, 2:51 p.m. | #3
Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

> Hello Marcin,
>
> Once again, thanks for this work!
>
> On Thu, 26 Jul 2018 19:15:23 +0200, Marcin Niestroj wrote:
>> When using uclibc libdevmapper.so was calling dm_task_get_info_base()
>> function recursively, leading to segmentation fault. This was
>> happening because uclibc linker loader just takes first existing
>> 'dm_task_get_info' (which is 'dm_task_get_info_base') symbol in elf
>> binary, instead of default version.
>> 
>> Add upstreamable lvm2 patch [1], which introduces
>> --enable-symvers[=STYLE] switch. Use that switch to disable symbol
>> versions, as we do not plan to support binaries compiled against
>> old libdevmapper library.
>> 
>> [1] https://www.redhat.com/archives/dm-devel/2018-July/msg00187.html
>> 
>> Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
>> ---
>> Changes v2 -> v3: none
>
> I have added a reference to the Buildroot bug report being fixed by
> this, and I've applied to master.
>
> Two questions:
>
>  (1) Could you try to push this forward in terms of upstreaming
>      acceptance ? I know you have submitted the patch, but I looked
>      today and apparently you haven't received any feedback.
>
>  (2) It would be nicer if the availability of symbol versioning could
>      be auto-detected. Is there a compile-time test that can be done to
>      verify if symbol versioning is available ? This would perhaps help
>      make the patch even more acceptable upstream.
>
> Thanks!
>
> Thomas
Marcin Niestroj Aug. 31, 2018, 3 p.m. | #4
Hi Thomas,

Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:
> <snip>
>
> I have added a reference to the Buildroot bug report being fixed by
> this, and I've applied to master.
>
> Two questions:
>
>  (1) Could you try to push this forward in terms of upstreaming
>      acceptance ? I know you have submitted the patch, but I looked
>      today and apparently you haven't received any feedback.

I was not available to do it earlier. I will take care of pushing it
forward.

>
>  (2) It would be nicer if the availability of symbol versioning could
>      be auto-detected. Is there a compile-time test that can be done to
>      verify if symbol versioning is available ? This would perhaps help
>      make the patch even more acceptable upstream.

Unfortunately I don't know how to check that. Symbol versioning is
something that runtime linker is aware of (glibc) or not (uclibc). So in
case of cross-compiling we are not able to check if library loads
properly. I have also not found any flags in libc libraries that
enable/disable symbol versioning, so external compiled against them
would know if that is supported.

For glibc and uclibc we could write some logic around __GLIBC__ and
__UCLIBC__ macros, but musl on the other hand does not allow to detect
it with any macro (which is done on purpose in musl).

>
> Thanks!
>
> Thomas

Patch

diff --git a/package/lvm2/0001-configure-Introduce-enable-symvers-option.patch b/package/lvm2/0001-configure-Introduce-enable-symvers-option.patch
new file mode 100644
index 0000000000..8ca39c2cab
--- /dev/null
+++ b/package/lvm2/0001-configure-Introduce-enable-symvers-option.patch
@@ -0,0 +1,277 @@ 
+From f563334a76e31442f7b8693d2d350e6981c51c46 Mon Sep 17 00:00:00 2001
+From: Marcin Niestroj <m.niestroj@grinn-global.com>
+Date: Fri, 20 Jul 2018 14:26:44 +0200
+Subject: [PATCH] configure: Introduce --enable-symvers option
+
+Only few libc (e.g. glibc) libraries support full symbol version
+resolution in runtime. There are lot of standard libraries that do not
+support that, such as dietlibc, musl and uclibc. Hence there is no
+reason to generate symbol versions when compiling against them.
+
+Additionally libdevmapper.so was broken when compiled against
+uclibc. Runtime linker loader caused calling dm_task_get_info_base()
+function recursively, leading to segmentation fault.
+
+Introduce --enable-symvers[=STYLE] option, which allows to choose
+between gnu and disabled symbol versioning. By default gnu symbol
+versioning is used to provide backward compatibility.
+__GNUC__ check is replaced now with GNU_SYMVER, which is generated by
+configure script. Additionally ld version script is included only in
+case of gnu option, which slightly reduces output size.
+
+Providing --disable-symvers to configure script when building against
+uclibc library fixes segmentation fault error described above, due to
+lack of several versions of the same symbol in libdevmapper.so
+library.
+
+Signed-off-by: Marcin Niestroj <m.niestroj@grinn-global.com>
+---
+ configure                 | 32 ++++++++++++++++++++++++++++++--
+ configure.in              | 28 +++++++++++++++++++++++++---
+ include/configure.h.in    |  3 +++
+ lib/misc/lib.h            | 10 +++++-----
+ libdm/datastruct/bitset.c |  5 +----
+ libdm/ioctl/libdm-iface.c |  2 +-
+ libdm/libdm-deptree.c     |  2 +-
+ libdm/libdm-stats.c       |  2 +-
+ 8 files changed, 67 insertions(+), 17 deletions(-)
+
+diff --git a/configure b/configure
+index e1ae0e884..c5d11c1b6 100755
+--- a/configure
++++ b/configure
+@@ -985,6 +985,7 @@ enable_fsadm
+ enable_blkdeactivate
+ enable_dmeventd
+ enable_selinux
++enable_symvers
+ enable_nls
+ with_localedir
+ with_confdir
+@@ -1729,6 +1730,9 @@ Optional Features:
+   --disable-blkdeactivate disable blkdeactivate
+   --enable-dmeventd       enable the device-mapper event daemon
+   --disable-selinux       disable selinux support
++  --enable-symvers[=STYLE]
++                          enables symbol versioning of the shared library
++                          [default=gnu]
+   --enable-nls            enable Native Language Support
+ 
+ Optional Packages:
+@@ -3169,7 +3173,6 @@ if test -z "$CFLAGS"; then :
+ fi
+ case "$host_os" in
+ 	linux*)
+-		CLDFLAGS="$CLDFLAGS -Wl,--version-script,.export.sym"
+ 		ELDFLAGS="-Wl,--export-dynamic"
+ 		# FIXME Generate list and use --dynamic-list=.dlopen.sym
+ 		CLDWHOLEARCHIVE="-Wl,-whole-archive"
+@@ -3190,7 +3193,6 @@ case "$host_os" in
+ 		;;
+ 	darwin*)
+ 		CFLAGS="$CFLAGS -no-cpp-precomp -fno-common"
+-		CLDFLAGS="$CLDFLAGS"
+ 		ELDFLAGS=
+ 		CLDWHOLEARCHIVE="-all_load"
+ 		CLDNOWHOLEARCHIVE=
+@@ -14609,6 +14611,32 @@ done
+ 	LIBS=$lvm_saved_libs
+ fi
+ 
++################################################################################
++{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to enable symbol versioning" >&5
++$as_echo_n "checking whether to enable symbol versioning... " >&6; }
++# Check whether --enable-symvers was given.
++if test "${enable_symvers+set}" = set; then :
++  enableval=$enable_symvers;
++  case "$enableval" in
++    gnu|no) ;;
++    *) as_fn_error $? "Unknown argument to enable/disable symvers" "$LINENO" 5 ;;
++  esac
++else
++  enable_symvers=gnu
++fi
++
++{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $enable_symvers" >&5
++$as_echo "$enable_symvers" >&6; }
++
++if test x$GCC = xyes && test x$enable_symvers = xgnu ; then
++
++$as_echo "#define GNU_SYMVER 1" >>confdefs.h
++
++  case "$host_os" in
++    linux*) CLDFLAGS="$CLDFLAGS -Wl,--version-script,.export.sym" ;;
++  esac
++fi
++
+ ################################################################################
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to enable internationalisation" >&5
+ $as_echo_n "checking whether to enable internationalisation... " >&6; }
+diff --git a/configure.in b/configure.in
+index 2e5e015c8..09c390850 100644
+--- a/configure.in
++++ b/configure.in
+@@ -30,12 +30,10 @@ AC_CANONICAL_TARGET([])
+ AS_IF([test -z "$CFLAGS"], [COPTIMISE_FLAG="-O2"])
+ case "$host_os" in
+ 	linux*)
+-		CLDFLAGS="$CLDFLAGS -Wl,--version-script,.export.sym"
+ 		ELDFLAGS="-Wl,--export-dynamic"
+ 		# FIXME Generate list and use --dynamic-list=.dlopen.sym
+ 		CLDWHOLEARCHIVE="-Wl,-whole-archive"
+ 		CLDNOWHOLEARCHIVE="-Wl,-no-whole-archive"
+-		LDDEPS="$LDDEPS .export.sym"
+ 		LIB_SUFFIX=so
+ 		DEVMAPPER=yes
+ 		BUILD_LVMETAD=no
+@@ -51,7 +49,6 @@ case "$host_os" in
+ 		;;
+ 	darwin*)
+ 		CFLAGS="$CFLAGS -no-cpp-precomp -fno-common"
+-		CLDFLAGS="$CLDFLAGS"
+ 		ELDFLAGS=
+ 		CLDWHOLEARCHIVE="-all_load"
+ 		CLDNOWHOLEARCHIVE=
+@@ -1742,6 +1739,31 @@ package as well (which may be called readline-devel or something similar).])
+ 	LIBS=$lvm_saved_libs
+ fi
+ 
++################################################################################
++dnl -- Symbol versioning
++AC_MSG_CHECKING(whether to enable symbol versioning)
++AC_ARG_ENABLE(symvers,
++              AC_HELP_STRING([--enable-symvers[[[=STYLE]]]],
++                             [enables symbol versioning of the shared library [default=gnu]]),
++                             [
++  case "$enableval" in
++    gnu|no) ;;
++    *) AC_MSG_ERROR(Unknown argument to enable/disable symvers) ;;
++  esac],
++                             enable_symvers=gnu)
++AC_MSG_RESULT($enable_symvers)
++
++if test x$GCC = xyes && test x$enable_symvers = xgnu ; then
++  AC_DEFINE(GNU_SYMVER, 1,
++            [Define to use GNU versioning in the shared library.])
++  case "$host_os" in
++    linux*)
++      CLDFLAGS="$CLDFLAGS -Wl,--version-script,.export.sym"
++      LDDEPS="$LDDEPS .export.sym"
++      ;;
++  esac
++fi
++
+ ################################################################################
+ dnl -- Internationalisation stuff
+ AC_MSG_CHECKING(whether to enable internationalisation)
+diff --git a/include/configure.h.in b/include/configure.h.in
+index 51726506c..3fc181b1e 100644
+--- a/include/configure.h.in
++++ b/include/configure.h.in
+@@ -151,6 +151,9 @@
+ /* Path to fsadm binary. */
+ #undef FSADM_PATH
+ 
++/* Define to use GNU versioning in the shared library. */
++#undef GNU_SYMVER
++
+ /* Define to 1 if you have the `alarm' function. */
+ #undef HAVE_ALARM
+ 
+diff --git a/lib/misc/lib.h b/lib/misc/lib.h
+index 8ed06f81d..9b3ce8a03 100644
+--- a/lib/misc/lib.h
++++ b/lib/misc/lib.h
+@@ -42,16 +42,16 @@
+  * macro DM_EXPORT_SYMBOL to export the function and bind it to the
+  * specified version string.
+  *
+- * Since versioning is only available when compiling with GCC the entire
+- * compatibility version should be enclosed in '#if defined(__GNUC__)',
+- * for example:
++ * Since versioning is only available when compiling with GCC
++ * and GLIBC the entire compatibility version should be enclosed
++ * in '#if defined(GNU_SYMVER)', for example:
+  *
+  *   int dm_foo(int bar)
+  *   {
+  *     return bar;
+  *   }
+  *
+- *   #if defined(__GNUC__)
++ *   #if defined(GNU_SYMVER)
+  *   // Backward compatible dm_foo() version 1.02.104
+  *   int dm_foo_v1_02_104(void);
+  *   int dm_foo_v1_02_104(void)
+@@ -68,7 +68,7 @@
+  * versions of library symbols prior to the introduction of symbol
+  * versioning: it must never be used for new symbols.
+  */
+-#if defined(__GNUC__)
++#if defined(GNU_SYMVER)
+ #define DM_EXPORT_SYMBOL(func, ver) \
+ 	__asm__(".symver " #func "_v" #ver ", " #func "@DM_" #ver )
+ #define DM_EXPORT_SYMBOL_BASE(func) \
+diff --git a/libdm/datastruct/bitset.c b/libdm/datastruct/bitset.c
+index b0826e1eb..2ec3f8f84 100644
+--- a/libdm/datastruct/bitset.c
++++ b/libdm/datastruct/bitset.c
+@@ -242,7 +242,7 @@ bad:
+ 	return NULL;
+ }
+ 
+-#if defined(__GNUC__)
++#if defined(GNU_SYMVER)
+ /*
+  * Maintain backward compatibility with older versions that did not
+  * accept a 'min_num_bits' argument to dm_bitset_parse_list().
+@@ -253,7 +253,4 @@ dm_bitset_t dm_bitset_parse_list_v1_02_129(const char *str, struct dm_pool *mem)
+ 	return dm_bitset_parse_list(str, mem, 0);
+ }
+ DM_EXPORT_SYMBOL(dm_bitset_parse_list, 1_02_129);
+-
+-#else /* if defined(__GNUC__) */
+-
+ #endif
+diff --git a/libdm/ioctl/libdm-iface.c b/libdm/ioctl/libdm-iface.c
+index c47e08467..b98afb15d 100644
+--- a/libdm/ioctl/libdm-iface.c
++++ b/libdm/ioctl/libdm-iface.c
+@@ -2137,7 +2137,7 @@ void dm_lib_exit(void)
+ 	_version_checked = 0;
+ }
+ 
+-#if defined(__GNUC__)
++#if defined(GNU_SYMVER)
+ /*
+  * Maintain binary backward compatibility.
+  * Version script mechanism works with 'gcc' compatible compilers only.
+diff --git a/libdm/libdm-deptree.c b/libdm/libdm-deptree.c
+index cf4fd62e7..474871da5 100644
+--- a/libdm/libdm-deptree.c
++++ b/libdm/libdm-deptree.c
+@@ -4110,7 +4110,7 @@ void dm_tree_node_set_callback(struct dm_tree_node *dnode,
+ 	dnode->callback_data = data;
+ }
+ 
+-#if defined(__GNUC__)
++#if defined(GNU_SYMVER)
+ /*
+  * Backward compatible implementations.
+  *
+diff --git a/libdm/libdm-stats.c b/libdm/libdm-stats.c
+index bc498675f..d424928c7 100644
+--- a/libdm/libdm-stats.c
++++ b/libdm/libdm-stats.c
+@@ -5064,7 +5064,7 @@ int dm_stats_start_filemapd(int fd, uint64_t group_id, const char *path,
+  * current dm_stats_create_region() version.
+  */
+ 
+-#if defined(__GNUC__)
++#if defined(GNU_SYMVER)
+ int dm_stats_create_region_v1_02_106(struct dm_stats *dms, uint64_t *region_id,
+ 				     uint64_t start, uint64_t len, int64_t step,
+ 				     int precise, const char *program_id,
+-- 
+2.18.0
+
diff --git a/package/lvm2/lvm2.mk b/package/lvm2/lvm2.mk
index 20e0dd0d5c..ccdc2c38e6 100644
--- a/package/lvm2/lvm2.mk
+++ b/package/lvm2/lvm2.mk
@@ -19,7 +19,8 @@  LVM2_CONF_OPTS += \
 	--enable-pkgconfig \
 	--enable-cmdlib \
 	--enable-dmeventd \
-	--disable-nls
+	--disable-nls \
+	--disable-symvers
 
 LVM2_DEPENDENCIES += host-pkgconf