Re: [PATCH net-next v3 1/2] bpf: Add bpf_current_task_in_cgroup helper
From: Daniel Borkmann <daniel@iogearbox.net>
Date: 2016-08-11 20:24:28
Hi Sargun, just some minor comment inline. On 08/11/2016 08:51 PM, Sargun Dhillon wrote: [...]
quoted hunk ↗ jump to hunk
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 1113423..6c01ab1 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h@@ -319,4 +319,26 @@ extern const struct bpf_func_proto bpf_get_stackid_proto; void bpf_user_rnd_init_once(void); u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5); +/* Helper to fetch a cgroup pointer based on index. + * @map: a cgroup arraymap + * @idx: index of the item you want to fetch + * + * Returns pointer on success, + * Error code if item not found, or out-of-bounds access + */ +static inline struct cgroup *fetch_arraymap_ptr(struct bpf_map *map, int idx) +{ + struct cgroup *cgrp; + struct bpf_array *array = container_of(map, struct bpf_array, map);
Nit, please keep it in this reverse tree like order (whenever possible) to be consistent with kernel coding style: struct bpf_array *array = container_of(map, struct bpf_array, map); struct cgroup *cgrp;
+ if (unlikely(idx >= array->map.max_entries)) + return ERR_PTR(-E2BIG); + + cgrp = READ_ONCE(array->ptrs[idx]); + if (unlikely(!cgrp)) + return ERR_PTR(-EAGAIN); + + return cgrp; +}
I think this inline helper doesn't buy us too much to be honest. First, it really should be prefixed with bpf_* if this is exposed like this to avoid potential naming clashes with other headers, but apart from that a generic function to fetch an array map pointer returning a cgroup is also not really generic. We have other specialized array maps also fetching a pointer apart from cgroup, so either we make a real generic helper for use in all of them, or just add the content of above into the bpf_current_task_in_cgroup() helper directly. Probably just going for the latter is more straight forward and likely also smaller diff. Rest looks otherwise good to me, thanks!