[PATCH] riscv: Correct __riscv_hwprobe function attributes [BZ #32932]
Mark Harris
mark.hsj@gmail.com
Mon May 5 23:43:21 GMT 2025
On Mon, May 5, 2025 at 6:11 AM enh <enh@google.com> wrote:
>
> On Fri, May 2, 2025 at 8:40 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 name of the third argument in the prototype (__cpu_count) is also
> > misleading since it is not a cpu count. This argument is passed to the
> > vDSO or kernel unchanged, and the vDSO, kernel, and kernel documentation
> > (Documentation/arch/riscv/hwprobe.rst) call this argument "cpusetsize"
> > and confirm that it is in bytes. Therefore rename this argument to
> > align with the kernel and clarify its expected value, and do not use it
> > as an element count in the access attribute.
>
> the kernel also has the bitmask as a `cpu_set_t*` rather than
> `unsigned long*` too, which seems clearer:
>
> long sys_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t pair_count,
> size_t cpusetsize, cpu_set_t *cpus,
> unsigned int flags);
>
> https://docs.kernel.org/arch/riscv/hwprobe.html
>
> should we fix that too while we're here?
cpu_set_t * would be consistent with other functions that take a cpu
bitmask (sched_setaffinity, sched_getaffinity, pthread_setaffinity_np,
pthread_getaffinity_np, pthread_attr_setaffinity_np,
pthread_attr_getaffinity_np), so it is unfortunate that it was added
with unsigned long *, but wouldn't changing it be a problem for any
existing code that is calling it with unsigned long *?
$ gcc -c hwprobe1.c
hwprobe1.c: In function 'main':
hwprobe1.c:15:52: error: passing argument 4 of '__riscv_hwprobe' from
incompatible pointer type [-Wincompatible-pointer-types]
15 | ret = __riscv_hwprobe (pairs, 1, sizeof(cpus), cpus, 0);
| ^~~~
| |
| long unsigned int *
In file included from hwprobe1.c:1:
hwprobe.h:68:61: note: expected 'cpu_set_t *' but argument is of type
'long unsigned int *'
68 | size_t __cpusetsize, cpu_set_t *__cpus,
| ~~~~~~~~~~~^~~~~~
$
- Mark
>
> > The (read_only, 4, 3) access attribute also claims that the cpu bitmask
> > is only read, however when flags is RISCV_HWPROBE_WHICH_CPUS it is both
> > read and written. Therefore the attribute is updated to read_write.
> >
> > 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 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).
> >
> > Signed-off-by: Mark Harris <mark.hsj@gmail.com>
> > ---
> > sysdeps/unix/sysv/linux/riscv/hwprobe.c | 4 ++--
> > sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h | 18 +++++++++---------
> > 2 files changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/sysdeps/unix/sysv/linux/riscv/hwprobe.c b/sysdeps/unix/sysv/linux/riscv/hwprobe.c
> > index e0cbd22cbe..f54a5d6c55 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, unsigned long int *cpus,
> > unsigned int flags)
> > {
> > int r;
> >
> > r = INTERNAL_VSYSCALL (riscv_hwprobe, 5, pairs, pair_count,
> > - cpu_count, cpus, flags);
> > + cpusetsize, cpus, 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..797ce02c48 100644
> > --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > @@ -64,21 +64,21 @@ 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,
> > + size_t __cpusetsize, unsigned long int *__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))
> > + __attr_access ((__read_write__, 4));
> >
> > -/* 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,
> > + size_t __cpusetsize, unsigned long int *__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))
> > + __attr_access ((__read_write__, 4));
> >
> > /* 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