Re: [PATCH 4/5] perf dso: Guard against cache underflow on short reads in dso_cache__memcpy()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Returning 0 from `dso_cache__memcpy()` causes a permanent EOF rather than triggering a cache miss and re-read as intended, breaking reads from dynamically growing files.

Pre-existing issues:
- [High] The RB-tree traversal in `__dso_cache__find()` is performed locklessly, creating a data race with concurrent cache insertions.
--

--- Patch [4]: [PATCH 4/5] perf dso: Guard against cache underflow on short reads in dso_cache__memcpy() ---
commit 06837abfd5cca9da5670ee54ec02e3164860900c
Author: Arnaldo Carvalho de Melo <[email protected]>

perf dso: Guard against cache underflow on short reads in dso_cache__memcpy()

dso_cache__memcpy() computes cache_size based on cache->size, but a short
pread can leave cache->size smaller than the RB tree lookup window. This
causes a subtraction wrap-around and an out-of-bounds memcpy when accessed
past the cached size. This patch attempts to fix it by returning 0 (cache
miss) when the offset falls outside the valid cached range so the caller
re-reads from the backing file.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 207f8744aac97..a0de56c93592a 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1002,7 +1002,17 @@ static ssize_t dso_cache__memcpy(struct dso_cache *cache, u64 offset, u8 *data,
>  				 u64 size, bool out)
>  {
>  	u64 cache_offset = offset - cache->offset;
> -	u64 cache_size   = min(cache->size - cache_offset, size);
> +	u64 cache_size;
> +
> +	/*
> +	 * The RB tree matches using DSO__DATA_CACHE_SIZE, but a short
> +	 * pread may leave cache->size smaller.  Treat an offset past
> +	 * the valid data as a cache miss so the caller re-reads.
> +	 */
> +	if (cache_offset >= cache->size)
> +		return 0;

[Severity: High]
Does returning 0 here actually trigger a re-read?

Looking at cached_io() which calls this function, it seems to treat a 0 return
value strictly as an EOF condition:

cached_io() {
    ...
    ret = dso_cache__memcpy(cache, offset, data, size, out);
    if (!ret)
        break;
    ...
}

If the file has grown and we hit this condition, won't this cause cached_io()
to terminate the loop with a silent EOF instead of reading the new data from
the backing file?

> +
> +	cache_size = min(cache->size - cache_offset, size);
>  
>  	if (out)
>  		memcpy(data, cache->data + cache_offset, cache_size);

[Severity: High]
This is a pre-existing issue, but I noticed a potential data race when looking
at the RB-tree lookups in this file.

In __dso_cache__find(), the RB-tree traversal is performed locklessly:

    while (*p != NULL) {
        ...
        cache = rb_entry(parent, struct dso_cache, rb_node);
        ...
        if (offset < cache->offset)
            p = &(*p)->rb_left;
        else if (offset >= end)
            p = &(*p)->rb_right;
    }

However, dso_cache__insert() modifies the RB-tree while correctly holding
the dso__lock(dso) mutex:

    mutex_lock(dso__lock(dso));
    ...
    rb_link_node(&new->rb_node, parent, p);
    rb_insert_color(&new->rb_node, root);

If a lockless reader in __dso_cache__find() traverses the tree concurrently
with a cache miss that triggers dso_cache__insert(), could the reader observe
inconsistent tree pointers during the rb_insert_color() rotations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4