[PATCH v2] riscv: Correct __riscv_hwprobe function prototype [BZ #32932]
Mark Harris
mark.hsj@gmail.com
Wed May 14 18:52:59 GMT 2025
On Tue, May 13, 2025 at 2:18 PM enh <enh@google.com> wrote:
> 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?
warn_unused_result could potentially catch a bug if added to any
non-void function, but when the simplest safe and correct usage does
not require the result, and ignoring the result does not result in any
resource leaks or other problems, the attribute is more likely to be
an annoyance. Especially in this case I would prefer to promote the
usage where the entire structure is initialized and so the result does
not require checking. Not only is it simpler, but all current uses
that I've seen will use syscall() directly if the libc function is not
available, which leads to an additional potential source of bugs if
the return value needs to be checked because those two interfaces use
a different convention for returning errors.
In fact that exact bug is present in the third-party cpuinfo code that
you referenced below; it checks for errors using "if (ret < 0)" but
__riscv_hwprobe returns a positive errno value on error. (And even
for syscall(), which returns -1 on error, the code is arguably wrong
since it logs the value of ret (-1) on error instead of getting the
error from errno where syscall() stores it.)
>
> (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 :-( )
That PR only removes the cast from the syscall(), not from
__riscv_hwprobe(), so if bionic __riscv_hwprobe() now requires
cpu_set_t * then won't the PR fail to build with the new bionic?
>
> > +#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
> >
- Mark
More information about the Libc-alpha
mailing list