Re: [PATCH 2/4] perf header: Use write lock when translating BPF prog info pointers

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

Pre-existing issues:
- [High] The patch changes `down_read` to `down_write` to protect in-place pointer mutations, but the fix is ineffective because primary readers access the data locklessly.
- [Medium] `bpil_offs_to_addr()` permanently corrupts the in-memory `info_linear->arrays` mask when restoring a valid object containing empty trailing arrays.
--

--- Patch [2]: [PATCH 2/4] perf header: Use write lock when translating BPF prog info pointers ---
commit b51042073da80b7c616b5e20db21243852bbccec
Author: Arnaldo Carvalho de Melo <[email protected]>

perf header: Use write lock when translating BPF prog info pointers

write_bpf_prog_info() temporarily mutates pointers into file offsets,
causing concurrent readers to dereference invalid addresses. This patch
upgrades the synchronization from a read lock to a write lock during the
translation window to exclude concurrent readers.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 091d8f7f6bd2c..70ba9570e3179 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -1120,7 +1120,8 @@ static int write_bpf_prog_info(struct feat_fd *ff  __maybe_unused,
>  	struct rb_node *next;
>  	int ret = 0;
>  
> -	down_read(&env->bpf_progs.lock);
> +	/* write lock: bpil_addr_to_offs() temporarily mutates info_linear */
> +	down_write(&env->bpf_progs.lock);

[Severity: High]
This is a pre-existing issue, but does upgrading to a write lock actually
prevent concurrent accesses from seeing the mutated pointers?

Looking at perf_env__find_bpf_prog_info(), it acquires the read lock but
drops it before returning the node pointer:

tools/perf/util/env.c:perf_env__find_bpf_prog_info() {
    ...
    up_read(&env->bpf_progs.lock);
    return node;
}

This means callers like machine__process_bpf_event_load() will dereference
the pointer locklessly:

tools/perf/util/bpf-event.c:machine__process_bpf_event_load() {
    ...
    u64 *addrs = (u64 *)(uintptr_t)(info_linear->info.jited_ksyms);
    ...
}

Because the primary readers are accessing the data outside the critical
section, could they still observe a file offset instead of a valid pointer
even with down_write() here?

>  
>  	ret = do_write(ff, &env->bpf_progs.infos_cnt,
>  		       sizeof(env->bpf_progs.infos_cnt));
> @@ -1150,7 +1151,7 @@ static int write_bpf_prog_info(struct feat_fd *ff  __maybe_unused,
>  			goto out;

[Severity: Medium]
This isn't a bug introduced by this patch, but could calling
bpil_offs_to_addr() just before this return permanently corrupt the state
for empty trailing arrays?

If a BPF program contains an empty array (count = 0) at the end of the
serialized struct, its offset is exactly data_len. When this function calls
bpil_offs_to_addr(node->info_linear) to reverse the offsets, it hits this
bounds check:

tools/perf/util/bpf-utils.c:bpil_offs_to_addr() {
    ...
    if (offs >= info_linear->data_len || ...)
        ...
        info_linear->arrays &= ~(1UL << i);
    ...
}

Since offs >= data_len evaluates to true, will this falsely mark an empty
array as absent in the arrays bitmask, corrupting the live in-memory object?

>  	}
>  out:
> -	up_read(&env->bpf_progs.lock);
> +	up_write(&env->bpf_progs.lock);
>  	return ret;

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