Re: [dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations

2023-09-14 Thread Benjamin Marzinski 
On Thu, Sep 14, 2023 at 04:51:31PM +0200, mwi...@suse.com wrote:
> From: Martin Wilck 
> 
> The use of uatomic_xchg() in alias.c causes a -Warray-bounds error
> on distributions using gcc 12, such as Fedora 37. This is a similar
> error to 2534c4f ("libmultipath: avoid -Warray-bounds error with gcc
> 12 and musl libc"). This happens only with liburcu 0.13 and earlier,
> and only with certain gcc versions. See liburcu commit 835b9ab
> ("Fix: x86 and s390 uatomic: __hp() macro warning with gcc 11").
> 
> Enhance the fix for 2534c4f by a adding a workaround for uatomic_xchg(),
> and introduce the macro URCU_VERSION (originally only used for multipathd)
> globally.
> 
Reviewed-by: Benjamin Marzinski 
> Signed-off-by: Martin Wilck 
> ---
>  Makefile.inc |  2 +-
>  create-config.mk |  5 +
>  libmultipath/alias.c |  5 +++--
>  libmultipath/lock.h  | 23 ++-
>  multipathd/Makefile  |  2 --
>  5 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/Makefile.inc b/Makefile.inc
> index 6e384e6..04bfa56 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -95,7 +95,7 @@ OPTFLAGS:= -O2 -g $(STACKPROT) --param=ssp-buffer-size=4
>  WARNFLAGS:= -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) 
> -Werror=implicit-int \
> -Werror=implicit-function-declaration -Werror=format-security 
> \
> $(WNOCLOBBERED) -Werror=cast-qual 
> $(ERROR_DISCARDED_QUALIFIERS) $(W_URCU_TYPE_LIMITS)
> -CPPFLAGS := $(FORTIFY_OPT) $(CPPFLAGS) \
> +CPPFLAGS := $(FORTIFY_OPT) $(CPPFLAGS) $(D_URCU_VERSION) \
>  -DBIN_DIR=\"$(bindir)\" -DMULTIPATH_DIR=\"$(plugindir)\" \
>  -DRUNTIME_DIR=\"$(runtimedir)\" 
> -DCONFIG_DIR=\"$(configdir)\" \
>  -DDEFAULT_CONFIGFILE=\"$(configfile)\" 
> -DSTATE_DIR=\"$(statedir)\" \
> diff --git a/create-config.mk b/create-config.mk
> index d125597..4d318b9 100644
> --- a/create-config.mk
> +++ b/create-config.mk
> @@ -73,6 +73,10 @@ TEST_URCU_TYPE_LIMITS = $(shell \
>   $(CC) -c -Werror=type-limits -o /dev/null -xc - 2>/dev/null  \
>   || echo -Wno-type-limits )
>  
> +URCU_VERSION = $(shell \
> + $(PKG_CONFIG) --modversion liburcu 2>/dev/null | \
> + awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 
> * $$1 + $$2) + $$3); }')
> +
>  DEFINES :=
>  
>  ifneq ($(call 
> check_func,dm_task_no_flush,$(devmapper_incdir)/libdevmapper.h),0)
> @@ -168,6 +172,7 @@ $(TOPDIR)/config.mk:  $(multipathdir)/autoconfig.h
>   @echo creating $@
>   @echo "FPIN_SUPPORT := $(FPIN_SUPPORT)" >$@
>   @echo "FORTIFY_OPT := $(FORTIFY_OPT)" >>$@
> + @echo "D_URCU_VERSION := $(call URCU_VERSION)" >>$@
>   @echo "SYSTEMD := $(SYSTEMD)" >>$@
>   @echo "ANA_SUPPORT := $(ANA_SUPPORT)" >>$@
>   @echo "STACKPROT := $(call 
> TEST_CC_OPTION,-fstack-protector-strong,-fstack-protector)" >>$@
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index e5d3f15..74431f3 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -24,6 +24,7 @@
>  #include "devmapper.h"
>  #include "strbuf.h"
>  #include "time-util.h"
> +#include "lock.h"
>  
>  /*
>   * significant parts of this file were taken from iscsi-bindings.c of the
> @@ -300,7 +301,7 @@ void handle_bindings_file_inotify(const struct 
> inotify_event *event)
>   pthread_mutex_unlock(_mutex);
>  
>   if (changed) {
> - uatomic_xchg(_file_changed, 1);
> + uatomic_xchg_int(_file_changed, 1);
>   condlog(3, "%s: bindings file must be re-read, new timestamp: 
> %ld.%06ld",
>   __func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
>   } else
> @@ -775,7 +776,7 @@ static int _read_bindings_file(const struct config *conf, 
> Bindings *bindings,
>   int rc = 0, ret, fd;
>   FILE *file;
>   struct stat st;
> - int has_changed = uatomic_xchg(_file_changed, 0);
> + int has_changed = uatomic_xchg_int(_file_changed, 0);
>  
>   if (!force) {
>   if (!has_changed) {
> diff --git a/libmultipath/lock.h b/libmultipath/lock.h
> index 9814be7..ac80d1d 100644
> --- a/libmultipath/lock.h
> +++ b/libmultipath/lock.h
> @@ -13,17 +13,22 @@ struct mutex_lock {
>   int waiters; /* uatomic access only */
>  };
>  
> -#if !defined(__GLIBC__) && defined(__GNUC__) && __GNUC__ == 12
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Warray-bounds"
> -#endif
> -
>  static inline void init_lock(struct mutex_lock *a)
>  {
>   pthread_mutex_init(>mutex, NULL);
>   uatomic_set(>waiters, 0);
>  }
>  
> +#if defined(__GNUC__) && __GNUC__ == 12 && URCU_VERSION < 0xe00
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Warray-bounds"
> +#endif
> +
> +static inline int uatomic_xchg_int(int *ptr, int val)
> +{
> + return uatomic_xchg(ptr, val);
> +}
> +
>  static inline void lock(struct mutex_lock *a)
>  {
>   uatomic_inc(>waiters);
> @@ -31,6 +36,10 @@

[dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations

2023-09-14 Thread mwilck 
From: Martin Wilck 

The use of uatomic_xchg() in alias.c causes a -Warray-bounds error
on distributions using gcc 12, such as Fedora 37. This is a similar
error to 2534c4f ("libmultipath: avoid -Warray-bounds error with gcc
12 and musl libc"). This happens only with liburcu 0.13 and earlier,
and only with certain gcc versions. See liburcu commit 835b9ab
("Fix: x86 and s390 uatomic: __hp() macro warning with gcc 11").

Enhance the fix for 2534c4f by a adding a workaround for uatomic_xchg(),
and introduce the macro URCU_VERSION (originally only used for multipathd)
globally.

Signed-off-by: Martin Wilck 
---
 Makefile.inc |  2 +-
 create-config.mk |  5 +
 libmultipath/alias.c |  5 +++--
 libmultipath/lock.h  | 23 ++-
 multipathd/Makefile  |  2 --
 5 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/Makefile.inc b/Makefile.inc
index 6e384e6..04bfa56 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -95,7 +95,7 @@ OPTFLAGS  := -O2 -g $(STACKPROT) --param=ssp-buffer-size=4
 WARNFLAGS  := -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) 
-Werror=implicit-int \
  -Werror=implicit-function-declaration -Werror=format-security 
\
  $(WNOCLOBBERED) -Werror=cast-qual 
$(ERROR_DISCARDED_QUALIFIERS) $(W_URCU_TYPE_LIMITS)
-CPPFLAGS   := $(FORTIFY_OPT) $(CPPFLAGS) \
+CPPFLAGS   := $(FORTIFY_OPT) $(CPPFLAGS) $(D_URCU_VERSION) \
   -DBIN_DIR=\"$(bindir)\" -DMULTIPATH_DIR=\"$(plugindir)\" \
   -DRUNTIME_DIR=\"$(runtimedir)\" 
-DCONFIG_DIR=\"$(configdir)\" \
   -DDEFAULT_CONFIGFILE=\"$(configfile)\" 
-DSTATE_DIR=\"$(statedir)\" \
diff --git a/create-config.mk b/create-config.mk
index d125597..4d318b9 100644
--- a/create-config.mk
+++ b/create-config.mk
@@ -73,6 +73,10 @@ TEST_URCU_TYPE_LIMITS = $(shell \
$(CC) -c -Werror=type-limits -o /dev/null -xc - 2>/dev/null  \
|| echo -Wno-type-limits )
 
+URCU_VERSION = $(shell \
+   $(PKG_CONFIG) --modversion liburcu 2>/dev/null | \
+   awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 
* $$1 + $$2) + $$3); }')
+
 DEFINES :=
 
 ifneq ($(call 
check_func,dm_task_no_flush,$(devmapper_incdir)/libdevmapper.h),0)
@@ -168,6 +172,7 @@ $(TOPDIR)/config.mk:$(multipathdir)/autoconfig.h
@echo creating $@
@echo "FPIN_SUPPORT := $(FPIN_SUPPORT)" >$@
@echo "FORTIFY_OPT := $(FORTIFY_OPT)" >>$@
+   @echo "D_URCU_VERSION := $(call URCU_VERSION)" >>$@
@echo "SYSTEMD := $(SYSTEMD)" >>$@
@echo "ANA_SUPPORT := $(ANA_SUPPORT)" >>$@
@echo "STACKPROT := $(call 
TEST_CC_OPTION,-fstack-protector-strong,-fstack-protector)" >>$@
diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index e5d3f15..74431f3 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -24,6 +24,7 @@
 #include "devmapper.h"
 #include "strbuf.h"
 #include "time-util.h"
+#include "lock.h"
 
 /*
  * significant parts of this file were taken from iscsi-bindings.c of the
@@ -300,7 +301,7 @@ void handle_bindings_file_inotify(const struct 
inotify_event *event)
pthread_mutex_unlock(_mutex);
 
if (changed) {
-   uatomic_xchg(_file_changed, 1);
+   uatomic_xchg_int(_file_changed, 1);
condlog(3, "%s: bindings file must be re-read, new timestamp: 
%ld.%06ld",
__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
} else
@@ -775,7 +776,7 @@ static int _read_bindings_file(const struct config *conf, 
Bindings *bindings,
int rc = 0, ret, fd;
FILE *file;
struct stat st;
-   int has_changed = uatomic_xchg(_file_changed, 0);
+   int has_changed = uatomic_xchg_int(_file_changed, 0);
 
if (!force) {
if (!has_changed) {
diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index 9814be7..ac80d1d 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -13,17 +13,22 @@ struct mutex_lock {
int waiters; /* uatomic access only */
 };
 
-#if !defined(__GLIBC__) && defined(__GNUC__) && __GNUC__ == 12
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Warray-bounds"
-#endif
-
 static inline void init_lock(struct mutex_lock *a)
 {
pthread_mutex_init(>mutex, NULL);
uatomic_set(>waiters, 0);
 }
 
+#if defined(__GNUC__) && __GNUC__ == 12 && URCU_VERSION < 0xe00
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+#endif
+
+static inline int uatomic_xchg_int(int *ptr, int val)
+{
+   return uatomic_xchg(ptr, val);
+}
+
 static inline void lock(struct mutex_lock *a)
 {
uatomic_inc(>waiters);
@@ -31,6 +36,10 @@ static inline void lock(struct mutex_lock *a)
uatomic_dec(>waiters);
 }
 
+#if defined(__GNUC__) && __GNUC__ == 12 && URCU_VERSION < 0xe00
+#pragma GCC diagnostic pop
+#endif
+
 static inline int trylock(struct mutex_lock *a)
 {
return pthread_mutex_trylock(>mutex);