| 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
| ||
|
Message-ID: <5657F544.1080708@huawei.com>
Date: Fri, 27 Nov 2015 14:16:36 +0800
From: "Wangnan (F)" <wangnan0@...wei.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
CC: <masami.hiramatsu.pt@...achi.com>, <ast@...nel.org>,
<lizefan@...wei.com>, <pi3orama@....com>,
<linux-kernel@...r.kernel.org>, Namhyung Kim <namhyung@...nel.org>
Subject: Re: [PATCH 04/16] bpf tools: Collect map definition in bpf_object
On 2015/11/27 4:56, Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 24, 2015 at 01:36:09PM +0000, Wang Nan escreveu:
[SNIP]
>> + }
>> return 0;
>> }
>>
>> @@ -688,37 +707,15 @@ static int
>> bpf_object__create_maps(struct bpf_object *obj)
>> {
>> unsigned int i;
>> - size_t nr_maps;
>> - int *pfd;
>> -
>> - nr_maps = obj->maps_buf_sz / sizeof(struct bpf_map_def);
>> - if (!obj->maps_buf || !nr_maps) {
>> - pr_debug("don't need create maps for %s\n",
>> - obj->path);
>> - return 0;
>> - }
>> -
>> - obj->map_fds = malloc(sizeof(int) * nr_maps);
> perhaps calloc?
This line is being removed, so you'll still see this in my next version...
>
>> - if (!obj->map_fds) {
>> - pr_warning("realloc perf_bpf_map_fds failed\n");
>> - return -ENOMEM;
>> - }
>> - obj->nr_map_fds = nr_maps;
>>
>> - /* fill all fd with -1 */
>> - memset(obj->map_fds, -1, sizeof(int) * nr_maps);
>> + for (i = 0; i < obj->nr_maps; i++) {
>> + struct bpf_map_def *def = &obj->maps[i].def;
>> + int *pfd = &obj->maps[i].fd;
>>
>> - pfd = obj->map_fds;
>> - for (i = 0; i < nr_maps; i++) {
>> - struct bpf_map_def def;
>> -
>> - def = *(struct bpf_map_def *)(obj->maps_buf +
>> - i * sizeof(struct bpf_map_def));
>> -
>> - *pfd = bpf_create_map(def.type,
>> - def.key_size,
>> - def.value_size,
>> - def.max_entries);
>> + *pfd = bpf_create_map(def->type,
>> + def->key_size,
>> + def->value_size,
>> + def->max_entries);
>> if (*pfd < 0) {
>> size_t j;
>> int err = *pfd;
>> @@ -726,22 +723,17 @@ bpf_object__create_maps(struct bpf_object *obj)
>> pr_warning("failed to create map: %s\n",
>> strerror(errno));
>> for (j = 0; j < i; j++)
>> - zclose(obj->map_fds[j]);
>> - obj->nr_map_fds = 0;
>> - zfree(&obj->map_fds);
>> + zclose(obj->maps[j].fd);
>> return err;
>> }
>> pr_debug("create map: fd=%d\n", *pfd);
>> - pfd++;
>> }
>>
>> - zfree(&obj->maps_buf);
>> - obj->maps_buf_sz = 0;
>> return 0;
>> }
>>
>> static int
>> -bpf_program__relocate(struct bpf_program *prog, int *map_fds)
>> +bpf_program__relocate(struct bpf_program *prog, struct bpf_object *obj)
>> {
>> int i;
>>
>> @@ -761,7 +753,7 @@ bpf_program__relocate(struct bpf_program *prog, int *map_fds)
>> return -LIBBPF_ERRNO__RELOC;
>> }
>> insns[insn_idx].src_reg = BPF_PSEUDO_MAP_FD;
>> - insns[insn_idx].imm = map_fds[map_idx];
>> + insns[insn_idx].imm = obj->maps[map_idx].fd;
>> }
>>
>> zfree(&prog->reloc_desc);
>> @@ -780,7 +772,7 @@ bpf_object__relocate(struct bpf_object *obj)
>> for (i = 0; i < obj->nr_programs; i++) {
>> prog = &obj->programs[i];
>>
>> - err = bpf_program__relocate(prog, obj->map_fds);
>> + err = bpf_program__relocate(prog, obj);
>> if (err) {
>> pr_warning("failed to relocate '%s'\n",
>> prog->section_name);
>> @@ -804,8 +796,7 @@ static int bpf_object__collect_reloc(struct bpf_object *obj)
>> Elf_Data *data = obj->efile.reloc[i].data;
>> int idx = shdr->sh_info;
>> struct bpf_program *prog;
>> - size_t nr_maps = obj->maps_buf_sz /
>> - sizeof(struct bpf_map_def);
>> + size_t nr_maps = obj->nr_maps;
>>
>> if (shdr->sh_type != SHT_REL) {
>> pr_warning("internal error at %d\n", __LINE__);
>> @@ -1050,10 +1041,8 @@ int bpf_object__unload(struct bpf_object *obj)
>> if (!obj)
>> return -EINVAL;
>>
>> - for (i = 0; i < obj->nr_map_fds; i++)
>> - zclose(obj->map_fds[i]);
>> - zfree(&obj->map_fds);
>> - obj->nr_map_fds = 0;
>> + for (i = 0; i < obj->nr_maps; i++)
>> + zclose(obj->maps[i].fd);
>>
>> for (i = 0; i < obj->nr_programs; i++)
>> bpf_program__unload(&obj->programs[i]);
>> @@ -1096,7 +1085,15 @@ void bpf_object__close(struct bpf_object *obj)
>> bpf_object__elf_finish(obj);
>> bpf_object__unload(obj);
>>
>> - zfree(&obj->maps_buf);
>> + for (i = 0; i < obj->nr_maps; i++) {
>> + if (obj->maps[i].clear_priv)
>> + obj->maps[i].clear_priv(&obj->maps[i],
>> + obj->maps[i].priv);
>> + obj->maps[i].priv = NULL;
>> + obj->maps[i].clear_priv = NULL;
>> + }
>> + zfree(&obj->maps);
>> + obj->nr_maps = 0;
>>
>> if (obj->programs && obj->nr_programs) {
>> for (i = 0; i < obj->nr_programs; i++)
>> @@ -1251,3 +1248,72 @@ int bpf_program__nth_fd(struct bpf_program *prog, int n)
>>
>> return fd;
>> }
>> +
>> +int bpf_map__get_fd(struct bpf_map *map)
>> +{
>> + if (!map)
>> + return -EINVAL;
>> +
>> + return map->fd;
>> +}
>> +
>> +int bpf_map__get_def(struct bpf_map *map, struct bpf_map_def *pdef)
>> +{
>> + if (!map || !pdef)
>> + return -EINVAL;
>> +
>> + *pdef = map->def;
>> + return 0;
>> +}
>> +
>> +int bpf_map__set_private(struct bpf_map *map, void *priv,
>> + bpf_map_clear_priv_t clear_priv)
>> +{
>> + if (!map)
>> + return -EINVAL;
>> +
>> + if (map->priv) {
>> + if (map->clear_priv)
>> + map->clear_priv(map, map->priv);
>> + }
>> +
>> + map->priv = priv;
>> + map->clear_priv = clear_priv;
>> + return 0;
>> +}
>> +
>> +int bpf_map__get_private(struct bpf_map *map, void **ppriv)
>> +{
>> + if (!map)
>> + return -EINVAL;
>> +
>> + if (ppriv)
>> + *ppriv = map->priv;
>> + return 0;
>> +}
>> +
>> +struct bpf_map *
>> +bpf_map__next(struct bpf_map *prev, struct bpf_object *obj)
>> +{
>> + size_t idx;
>> + struct bpf_map *s, *e;
>> +
>> + if (!obj || !obj->maps)
>> + return NULL;
>> +
>> + s = obj->maps;
>> + e = obj->maps + obj->nr_maps;
>> +
>> + if (prev == NULL)
>> + return s;
>> +
>> + if ((prev < s) || (prev >= e)) {
>> + pr_warning("error: map handler doesn't belong to object\n");
> I wonder if this shouldn't be made pr_debug, and as well have a function
> prefix, otherwise we may think this is related to some other kind of
> map, so I suggest:
> pr_debug("%s: error: map handler doesn't belong to object\n", __func__);
>
> Or at least:
>
> pr_debug("BPF error: map handler doesn't belong to object\n");
We always have a libbpf prefix before debug info from libbpf. Please see
static __printf(1, 2) libbpf_print_fn_t __pr_warning = __base_pr;
static __printf(1, 2) libbpf_print_fn_t __pr_info = __base_pr;
static __printf(1, 2) libbpf_print_fn_t __pr_debug;
#define __pr(func, fmt, ...) \
do { \
if ((func)) \
(func)("libbpf: " fmt, ##__VA_ARGS__); \
} while (0)
#define pr_warning(fmt, ...) __pr(__pr_warning, fmt, ##__VA_ARGS__)
#define pr_info(fmt, ...) __pr(__pr_info, fmt, ##__VA_ARGS__)
#define pr_debug(fmt, ...) __pr(__pr_debug, fmt, ##__VA_ARGS__)
We allow __pr_{warning,info,debug} be overwritten but the 'libbpf:'
prefix is
always there.
Also, don't forget in perf's context libbpf is muted.
>> + return NULL;
>> + }
>> +
>> + idx = (prev - obj->maps) + 1;
>> + if (idx >= obj->nr_maps)
>> + return NULL;
>> + return &obj->maps[idx];
>> +}
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 949df4b..709d2fa 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -165,4 +165,25 @@ struct bpf_map_def {
>> unsigned int max_entries;
>> };
>>
>> +/*
>> + * There is another 'struct bpf_map' in include/linux/map.h. However,
>> + * it is not a uapi header so no need to consider name confliction.
> s/confliction/conflict/g
>
> But I would use "name clash".
Good word.
Thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists