netdev - Re: [PATCH bpf-next 1/2] bpf: introduce BPF_F_SHARE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Sep 2020 21:18:13 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Song Liu <songliubraving@...com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, kernel-team@...com, ast@...nel.org,
        john.fastabend@...il.com, kpsingh@...omium.org
Subject: Re: [PATCH bpf-next 1/2] bpf: introduce BPF_F_SHARE_PE for perf event
 array

On 9/29/20 9:00 PM, Alexei Starovoitov wrote:
> On Tue, Sep 29, 2020 at 04:02:10PM +0200, Daniel Borkmann wrote:
>>> +
>>> +/* Share perf_event among processes */
>>> +	BPF_F_SHARE_PE		= (1U << 11),
>>
>> nit but given UAPI: maybe name into something more self-descriptive
>> like BPF_F_SHAREABLE_EVENT ?
> 
> I'm not happy with either name.
> It's not about sharing and not really about perf event.
> I think the current behavior of perf_event_array is unusual and surprising.
> Sadly we cannot fix it without breaking user space, so flag is needed.
> How about BPF_F_STICKY_OBJECTS or BPF_F_PRESERVE_OBJECTS
> or the same with s/OBJECTS/FILES/ ?

Sounds good to me, BPF_F_PRESERVE_OBJECTS or _ENTRIES seems reasonable.

>>> +static void perf_event_fd_array_map_free(struct bpf_map *map)
>>> +{
>>> +	struct bpf_event_entry *ee;
>>> +	struct bpf_array *array;
>>> +	int i;
>>> +
>>> +	if ((map->map_flags & BPF_F_SHARE_PE) == 0) {
>>> +		fd_array_map_free(map);
>>> +		return;
>>> +	}
>>> +
>>> +	array = container_of(map, struct bpf_array, map);
>>> +	for (i = 0; i < array->map.max_entries; i++) {
>>> +		ee = READ_ONCE(array->ptrs[i]);
>>> +		if (ee)
>>> +			fd_array_map_delete_elem(map, &i);
>>> +	}
>>> +	bpf_map_area_free(array);
>>
>> Why not simplify into:
>>
>> 	if (map->map_flags & BPF_F_SHAREABLE_EVENT)
>> 		bpf_fd_array_map_clear(map);
>> 	fd_array_map_free(map);
> 
> +1
> 
>>> +}
>>> +
>>>    static void *prog_fd_array_get_ptr(struct bpf_map *map,
>>>    				   struct file *map_file, int fd)
>>>    {
>>> @@ -1134,6 +1158,9 @@ static void perf_event_fd_array_release(struct bpf_map *map,
>>>    	struct bpf_event_entry *ee;
>>>    	int i;
> 
> add empty line pls.
> 
>>> +	if (map->map_flags & BPF_F_SHARE_PE)
>>> +		return;
>>> +