[PATCH] riscv: Correct __riscv_hwprobe function attributes [BZ #32932]

[enh]

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?

> 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
>