[PATCH] riscv: Correct __riscv_hwprobe function attributes [BZ #32932]
enh
enh@google.com
Tue May 6 12:32:35 GMT 2025
On Mon, May 5, 2025 at 7:43 PM Mark Harris <mark.hsj@gmail.com> wrote:
>
> 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 *?
yeah, but that will only get worse...
https://github.com/pytorch/cpuinfo/blob/main/src/riscv/linux/riscv-hw.c
is the only caller i could find that doesn't just pass null. (and
amusingly it's not using <sys/hwprobe.h> for glibc [or musl, though
that doesn't have the header yet].)
fweimer? any precedent for fixing an intention-obscuring function
declaration (so it matches the kernel)?
> $ 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