Re: [PATCH bpf-next v5 5/9] bpftool: Generate skeleton for global percpu data

[Leon Hwang <leon.hwang@linux.dev>]

On 8/6/26 23:11, sashiko-bot@kernel.org wrote:
[...]
>> @@ -263,13 +268,12 @@ static bool is_mmapable_map(const struct bpf_map *map, char *buf, size_t sz)
>>  		return true;
>>  	}
>>  
>> -	if (!bpf_map__is_internal(map) || !(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
>> -		return false;
>> -
>> -	if (!get_map_ident(map, buf, sz))
>> -		return false;
>> +	if (bpf_map__is_internal(map) &&
>> +	    ((bpf_map__map_flags(map) & BPF_F_MMAPABLE) || bpf_map_is_percpu_data(map)) &&
>> +	    get_map_ident(map, buf, sz))
>> +		return true;
>>  
>> -	return true;
>> +	return false;
>>  }
> 
> [Severity: High]
> Does forcibly treating the percpu maps as mmapable here break subskeletons?
> 
> Since subskeletons operate on an already-loaded BPF object to read or write
> live state, this change might cause do_subskeleton() to export pointers to
> disconnected user-space memory.
> 
> If they point to the disconnected PROT_READ initialization buffer instead of
> live state, would reads always return stale initial data and fail to reflect
> live kernel modifications?
> 

Do subskeletons operate on an already-loaded BPF object to read or write
live state?

If true, will skip percpu data variables for subskeleton.

> [ ... ]
> 
>> @@ -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)
> 
> [Severity: High]
> Does skipping map finalization for percpu maps in light skeletons leave the
> user-space initialization buffer mapped as PROT_READ | PROT_WRITE?
> 
> In standard skeletons managed by libbpf, the buffer is protected with
> PROT_READ to explicitly crash on invalid writes.
> 
> If we skip finalization here in gen_trace(), it seems writes to lskel->percpu
> after load() might silently succeed without actually updating the kernel map.
> 

The buffer should be protected with PROT_READ in light skeletons. Will
do it.

Thanks,
Leon