Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1191380 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/3] bpf: Add new bpf map type to store the pointer to  struct perf_event Alexei Starovoitov <ast@plumgrid.com> - 2015-07-24 01:00 +0200
    Re: [PATCH v3 1/3] bpf: Add new bpf map type to store the pointer to  struct perf_event Alexei Starovoitov <ast@plumgrid.com> - 2015-07-24 04:30 +0200
    Re: [PATCH v3 1/3] bpf: Add new bpf map type to store the pointer  to struct perf_event xiakaixu <xiakaixu@huawei.com> - 2015-07-24 04:30 +0200

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

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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1191454

On 7/23/15 7:22 PM, xiakaixu wrote:
>>> +    /* 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.

I see then it's even worse.
You think that above check:
+    if (array->events[index])
+        return -EINVAL;
will protect the double insert?
It won't, since there are no locks here.
You can have two processes both seeing empty slot and
racing to do xchg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

于 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.
> 
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web