Re: [PATCH v1] libperf mmap: In user mmap rdpmc avoid undefined behavior

[Rob Herring <robh@kernel.org>]

On Sun, Sep 14, 2025 at 11:24:37AM -0700, Ian Rogers wrote:
> A shift left of a signed 64-bit s64 may overflow and result in
> undefined behavior caught by ubsan. Switch to a u64 instead.

This change was just brought to my attention...

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/perf/mmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/lib/perf/mmap.c b/tools/lib/perf/mmap.c
> index c1a51d925e0e..ec124eb0ec0a 100644
> --- a/tools/lib/perf/mmap.c
> +++ b/tools/lib/perf/mmap.c
> @@ -508,7 +508,7 @@ int perf_mmap__read_self(struct perf_mmap *map, struct perf_counts_values *count
>  		idx = READ_ONCE(pc->index);
>  		cnt = READ_ONCE(pc->offset);
>  		if (pc->cap_user_rdpmc && idx) {
> -			s64 evcnt = read_perf_counter(idx - 1);
> +			u64 evcnt = read_perf_counter(idx - 1);

According to uapi/linux/perf_event.h, this needs to be signed to sign 
extend the counter value:

        /*
         * If cap_user_rdpmc this field provides the bit-width of the value
         * read using the rdpmc() or equivalent instruction. This can be used
         * to sign extend the result like:
         *
         *   pmc <<= 64 - width;
         *   pmc >>= 64 - width; // signed shift right
         *   count += pmc;
         */

It looks like a left/right shift has gone from implementation defined to 
undefined behavior. According to the internet, the right way would be 
something like this:

u64 sign_bit = 1ULL << (width - 1);
s64 val64 = (s64)(((raw & ((1ULL << width) - 1)) ^ sign_bit) - sign_bit);

>  			u16 width = READ_ONCE(pc->pmc_width);
>  
>  			evcnt <<= 64 - width;

The left and then right shift here is probably redundant. I suppose 
there could be garbage in the upper bits to zero out, but I think 
read_perf_counter() should ensure there isn't.

Did the need for signed values go away or was wrong to begin with?

At a minimum, the header comments which documents the proper sequence no 
longer matches the implementation.

Rob