[PATCH v2] riscv: Correct __riscv_hwprobe function prototype [BZ #32932]
enh
enh@google.com
Tue May 13 21:18:01 GMT 2025
On Wed, May 7, 2025 at 3:19 PM Mark Harris <mark.hsj@gmail.com> wrote:
>
> The third argument to __riscv_hwprobe is the size in bytes of the
> cpu bitmask pointed to by the fourth argument, however in the access
> attribute (read_only, 4, 3) it is used as an element count (i.e., the
> number of unsigned longs that make up the bitmask), resulting in a
> false compiler warning:
>
> $ gcc -c hwprobe1.c
> hwprobe1.c: In function 'main':
> hwprobe1.c:15:11: warning: '__riscv_hwprobe' reading 1024 bytes from a region of size 128 [-Wstringop-overread]
> 15 | ret = __riscv_hwprobe (pairs, 1, sizeof(cpus), cpus, 0);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> hwprobe1.c:9:23: note: source object 'cpus' of size 128
> 9 | unsigned long int cpus[16];
> | ^~~~
> In file included from hwprobe1.c:1:
> /usr/include/riscv64-linux-gnu/sys/hwprobe.h:66:12: note: in a call to function '__riscv_hwprobe' declared with attribute 'access (read_only, 4, 3)'
> 66 | extern int __riscv_hwprobe (struct riscv_hwprobe *__pairs, size_t __pair_count,
> | ^~~~~~~~~~~~~~~
> $
>
> The documentation (https://docs.kernel.org/arch/riscv/hwprobe.html)
> claims that the cpu bitmask has the type cpu_set_t *, which would be
> consistent with other functions that take a cpu bitmask such as
> sched_setaffinity and sched_getaffinity. It also uses the name
> cpusetsize for the third argument, which is much more accurate than
> cpu_count since it is a size in bytes and not a cpu count. The
> (read_only, 4, 3) access attribute in the glibc prototype claims
> that the cpu bitmask is only read, however when flags is
> RISCV_HWPROBE_WHICH_CPUS it is both read and written.
>
> Therefore, in the glibc prototype the type of the fourth argument is
> changed to cpu_set_t * to match the documentation, the name of the
> third argument is changed to cpusetsize as in the documention, and the
> incorrect access attribute that applies to these arguments is removed.
> Almost all existing callers pass a null pointer for the fourth
> argument, however a transparent union is introduced for compatibility
> with callers that cast a pointer to the old argument type, and a
> macro is introduced allowing callers the ability to distinguish
> between the old and new prototype when needed.
>
> The access attributes are being specified with __fortified_attr_access,
> however this macro is for fortified functions; the regular
> __attr_access macro is for non-fortified functions such as this one.
> Using the incorrect macro results in no access checks at fortify level
> 3, because it is assumed that the fortified function will be doing the
> checking. It is changed to use the correct macro so that the access
> checks will work regardless of fortify level.
>
> Also because __riscv_hwprobe is not a cancellation point, __THROW
> is added, consistent with similar functions. (However, it is omitted
> from the typedef because GCC does not accept it there.)
>
> The __wur (warn_unused_result) attribute is helpful for functions that
> cannot be used safely without checking the result, however there is
> no such issue with __riscv_hwprobe; code such as the following should
> not produce a warning:
> struct riscv_hwprobe pair = { RISCV_HWPROBE_KEY_IMA_EXT_0, 0 };
> __riscv_hwprobe (&pair, 1, 0, NULL, 0);
> if (pair.value & RISCV_HWPROBE_EXT_ZBB) ...
> Therefore this attribute is omitted.
the similar
struct riscv_hwprobe pair;
pair.key = RISCV_HWPROBE_KEY_IMA_EXT_0;
would be wrong though?
(actually, now you've made me look, i've found that one of the callers
i have indexed is doing exactly this. though the fact that they assign
to a "ret" that they then ignore probably means __wur wouldn't have
kept them out of mischief either.)
> The comment claiming that the second argument to the ifunc selector
> is a pointer to the vDSO function is corrected. It is a pointer to
> the regular glibc function (which returns errors as positive values),
> not the vDSO function (which returns errors as negative values).
>
> Fixes commit 426d0e1aa8f17426d13707594111df712d2b8911 ("riscv: Add
> Linux hwprobe syscall support").
>
> Fixes: BZ #32932
> Signed-off-by: Mark Harris <mark.hsj@gmail.com>
> ---
> Changes in v2:
> - Type of __riscv_hwprobe fourth argument changed (suggested by enh).
> - Compatibility for C callers using the old argument type and new
> macro to allow detection of old vs. new argument type.
> - Include <sched.h> from <sys/hwprobe.h> for cpu_set_t.
> - Remove access attribute for fourth argument because it is valid to
> pass a pointer obtained from CPU_ALLOC that points to fewer bytes
> than the fixed-size cpu_set_t type.
>
> ---
> sysdeps/unix/sysv/linux/riscv/hwprobe.c | 4 +--
> sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h | 40 +++++++++++++++------
> 2 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/riscv/hwprobe.c b/sysdeps/unix/sysv/linux/riscv/hwprobe.c
> index e0cbd22cbe..bc7f6f38e1 100644
> --- a/sysdeps/unix/sysv/linux/riscv/hwprobe.c
> +++ b/sysdeps/unix/sysv/linux/riscv/hwprobe.c
> @@ -23,13 +23,13 @@
> #include <sysdep-vdso.h>
>
> int __riscv_hwprobe (struct riscv_hwprobe *pairs, size_t pair_count,
> - size_t cpu_count, unsigned long int *cpus,
> + size_t cpusetsize, __RISCV_HWPROBE_CPUS_TYPE cpus,
> unsigned int flags)
> {
> int r;
>
> r = INTERNAL_VSYSCALL (riscv_hwprobe, 5, pairs, pair_count,
> - cpu_count, cpus, flags);
> + cpusetsize, cpus.__ul, flags);
>
> /* Negate negative errno values to match pthreads API. */
> return -r;
> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> index bebad6cf70..40415aae80 100644
> --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> @@ -21,6 +21,7 @@
> #define _SYS_HWPROBE_H 1
>
> #include <features.h>
> +#include <sched.h>
> #include <stddef.h>
> #include <errno.h>
> #ifdef __has_include
> @@ -63,22 +64,39 @@ struct riscv_hwprobe {
>
> __BEGIN_DECLS
>
> -extern int __riscv_hwprobe (struct riscv_hwprobe *__pairs, size_t __pair_count,
> - size_t __cpu_count, unsigned long int *__cpus,
> +#if defined __cplusplus || !__GNUC_PREREQ (2, 7)
> +# define __RISCV_HWPROBE_CPUS_TYPE cpu_set_t *
> +#else
> +/* The fourth argument to __riscv_hwprobe should be a null pointer or a
> + pointer to a cpu_set_t (either the fixed-size type or allocated with
> + CPU_ALLOC). However, early versions of this header file used the
> + argument type unsigned long int *. The transparent union allows
> + the argument to be either cpu_set_t * or unsigned long int * for
> + compatibility. The older header file requiring unsigned long int *
> + can be identified by the lack of the __RISCV_HWPROBE_CPUS_TYPE macro.
> + In C++ and with compilers that do not support transparent unions, the
> + argument type must be cpu_set_t *. */
> +typedef union {
> + cpu_set_t *__cs;
> + unsigned long int *__ul;
> +} __RISCV_HWPROBE_CPUS_TYPE __attribute__ ((__transparent_union__));
> +# define __RISCV_HWPROBE_CPUS_TYPE __RISCV_HWPROBE_CPUS_TYPE
(fwiw, for bionic i've gone with just changing to cpu_set_t* for
everyone, on the basis of "the sooner we break it, the sooner it gets
fixed". given that i've only found one caller. i've sent them a
preparatory patch removing the cast they _never_ needed
[https://github.com/pytorch/cpuinfo/pull/292] but no response to that
so far :-( )
> +#endif
> +
> +extern int __riscv_hwprobe (struct riscv_hwprobe *__pairs,
> + size_t __pair_count, size_t __cpusetsize,
> + __RISCV_HWPROBE_CPUS_TYPE __cpus,
> unsigned int __flags)
> - __nonnull ((1)) __wur
> - __fortified_attr_access (__read_write__, 1, 2)
> - __fortified_attr_access (__read_only__, 4, 3);
> + __THROW __nonnull ((1)) __attr_access ((__read_write__, 1, 2));
>
> -/* A pointer to the __riscv_hwprobe vDSO function is passed as the second
> +/* A pointer to the __riscv_hwprobe function is passed as the second
> argument to ifunc selector routines. Include a function pointer type for
> convenience in calling the function in those settings. */
> -typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs, size_t __pair_count,
> - size_t __cpu_count, unsigned long int *__cpus,
> +typedef int (*__riscv_hwprobe_t) (struct riscv_hwprobe *__pairs,
> + size_t __pair_count, size_t __cpusetsize,
> + __RISCV_HWPROBE_CPUS_TYPE __cpus,
> unsigned int __flags)
> - __nonnull ((1)) __wur
> - __fortified_attr_access (__read_write__, 1, 2)
> - __fortified_attr_access (__read_only__, 4, 3);
> + __nonnull ((1)) __attr_access ((__read_write__, 1, 2));
>
> /* Helper function usable from ifunc selectors that probes a single key. */
> static __inline int
> --
> 2.49.0
>
More information about the Libc-alpha
mailing list