Messages in this thread | | | Subject | Re: [PATCH v3 1/3] bpf: Add new bpf map type to store the pointer to struct perf_event | From | Alexei Starovoitov <> | Date | Thu, 23 Jul 2015 15:54:36 -0700 |
| |
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.
> +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?
| |