Re: [PATCH v3 1/3] bpf: Add new bpf map type to store the pointer to struct perf_event

xiakaixu
2015-07-24T02:23:08Z

于 2015/7/24 6:54, Alexei Starovoitov 写道:
> On 7/23/15 2:42 AM, Kaixu Xia wrote:
>> Introduce a new bpf map type 'BPF_MAP_TYPE_PERF_EVENT_ARRAY'.
>> This map only stores the pointer to struct perf_event. The
>> user space event FDs from perf_event_open() syscall are converted
>> to the pointer to struct perf_event and stored in map.
> ...
>> +static struct bpf_map *perf_event_array_map_alloc(union bpf_attr *attr)
>> +{
>> + /* only the pointer to struct perf_event can be stored in
>> + * perf_event_array map
>> + */
>> + if (attr->value_size != sizeof(u32))
>> + return ERR_PTR(-EINVAL);
>> +
>> + return array_map_alloc(attr);
>> +}
>
> since it's exactly the same as prog_array_map_alloc(),
> just rename it to something like 'fd_array_map_alloc'
> and use for both types.
>
>> +static int perf_event_array_map_get_next_key(struct bpf_map *map, void *key,
>> + void *next_key)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +static void *perf_event_array_map_lookup_elem(struct bpf_map *map, void *key)
>> +{
>> + return NULL;
>> +}
>
> same for the above two.
> rename prog_array_map_* into fd_array_map_* and use for both map types.
>
>> +static struct perf_event *convert_map_with_perf_event(void *value)
>> +{
>> + struct perf_event *event;
>> + u32 fd;
>> +
>> + fd = *(u32 *)value;
>> +
>> + event = perf_event_get(fd);
>> + if (IS_ERR(event))
>> + return NULL;
>
> don't lose error code, do 'return event' instead.
>
>> +
>> + /* limit the event type to PERF_TYPE_RAW
>> + * and PERF_TYPE_HARDWARE.
>> + */
>> + if (event->attr.type != PERF_TYPE_RAW &&
>> + event->attr.type != PERF_TYPE_HARDWARE)
>> + return NULL;
>
> perf_event refcnt leak? need to do put_event.
> and return ERR_PTR(-EINVAL)
>
>> +
>> + return event;
>> +}
>> +
>> +/* only called from syscall */
>> +static int perf_event_array_map_update_elem(struct bpf_map *map, void *key,
>> + void *value, u64 map_flags)
>> +{
>> + struct bpf_array *array = container_of(map, struct bpf_array, map);
>> + struct perf_event *event;
>> + u32 index = *(u32 *)key;
>> +
>> + if (map_flags != BPF_ANY)
>> + return -EINVAL;
>> +
>> + if (index >= array->map.max_entries)
>> + return -E2BIG;
>> +
>> + /* check if the value is already stored */
>> + if (array->events[index])
>> + return -EINVAL;
>> +
>> + /* convert the fd to the pointer to struct perf_event */
>> + event = convert_map_with_perf_event(value);
>
> imo helper name is misleading and it's too short to be separate
> function. Just inline it and you can reuse 'index' variable.
>
>> + if (!event)
>> + return -EBADF;
>> +
>> + xchg(array->events + index, event);
>
> refcnt leak of old event! Please think it through.
> This type of bugs I shouldn't be finding.

Maybe the commit message is not elaborate. Here I prevent
user space from updating the existed event, so the return
value of xchg() is NULL and no refcnt leak of old event.
I will do the same as prog_array in next version.
>
>> +static int perf_event_array_map_delete_elem(struct bpf_map *map, void *key)
>> +{
>> + return -EINVAL;
>> +}
>
> no way to dec refcnt of perf_event from user space?
> why not to do the same as prog_array_delete?

Will follow them in V4.
>
>
> .
>