Re: [PATCH bpf-next v4 6/8] bpftool: Generate skeleton for global percpu data

On 15/4/26 05:26, [email protected] wrote:
> Sashiko AI review found 1 potential issue(s):
> - [High] Missing read-only memory protection for `.percpu` initial value buffer in generated light skeletons.
> --
> 
> commit e61b5ee6e5a7cee758b644de0f6b4aaea40f020c
> Author: Leon Hwang <[email protected]>
> 
> bpftool: Generate skeleton for global percpu data
> 
> Enhance bpftool to generate skeletons that properly handle global percpu
> variables. The skeleton includes a nested structure for percpu data, which
> is marked as read-only after loading so users must update the map directly.
> 
>> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
>> --- a/tools/bpf/bpftool/gen.c
>> +++ b/tools/bpf/bpftool/gen.c
> 
> [ ... ]
> 
>> @@ -847,7 +851,7 @@ static int gen_trace(struct bpf_object *obj, const char *obj_name, const char *h
>>  	bpf_object__for_each_map(map, obj) {
>>  		const char *mmap_flags;
>>  
>> -		if (!is_mmapable_map(map, ident, sizeof(ident)))
>> +		if (!is_mmapable_map(map, ident, sizeof(ident)) || bpf_map_is_percpu_data(map))
>>  			continue;
>>  
>>  		if (bpf_map__map_flags(map) & BPF_F_RDONLY_PROG)
> 
> The commit message notes that after loading the skeleton, maps.percpu->mmaped
> has been marked as read-only.
> 
> By skipping the .percpu map entirely here during light skeleton generation
> in gen_trace(), does this leave the user-space buffer for .percpu writable?
> 
> If a user modifies skel->percpu after the light skeleton is loaded, will it
> silently succeed without actually updating the kernel per-cpu map? Does the
> generated light skeleton need an explicit mprotect(..., PROT_READ) for the
> .percpu initial value buffer to enforce the read-only constraint?
> 

The mprotect(..., PROT_READ) was added in patch #4.

I think it'd better to move the change to this patch. Then, readers
won't miss the mprotect().

Thanks,
Leon