Re: [RFC PATCH v3 16/37] bpf tools: Collect eBPF programs from their own sections

["Wangnan (F)" <wangnan0@huawei.com>]

在 2015/5/18 20:29, Namhyung Kim 写道:
> Hi,
>
> On Sun, May 17, 2015 at 10:56:41AM +0000, Wang Nan wrote:
>> This patch collects all programs in an object file into an array of
>> 'struct bpf_program' for further processing. That structure is for
>> representing each eBPF program. 'bpf_prog' should be a better name, but
>> it has been used by linux/filter.h. Although it is a kernel space name,
>> I still prefer to call it 'bpf_program' to prevent possible confusion.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> ---
> [SNIP]
>
>> +static struct bpf_program *
>> +bpf_program_alloc(struct bpf_object *obj, void *data, size_t size,
>> +		  char *name, int idx)
>> +{
>> +	struct bpf_program *prog, *progs;
>> +	int nr_progs;
>> +
>> +	if (size < sizeof(struct bpf_insn)) {
>> +		pr_warning("corrupted section '%s'\n", name);
>> +		return NULL;
>> +	}
>> +	
>> +	progs = obj->programs;
>> +	nr_progs = obj->nr_programs;
>> +
>> +	progs = realloc(progs, sizeof(*prog) * (nr_progs + 1));
>> +	if (!progs) {
>> +		/*
>> +		 * In this case the original obj->programs
>> +		 * is still valid, so don't need special treat for
>> +		 * bpf_close_object().
>> +		 */
>> +		pr_warning("failed to alloc a new program '%s'\n",
>> +			   name);
>> +		return NULL;
>> +	}
>> +
>> +	obj->programs = progs;
>> +
>> +	prog = &progs[nr_progs];
>> +	bzero(prog, sizeof(*prog));
>> +
>> +	obj->nr_programs = nr_progs + 1;
>> +
>> +	prog->section_name = strdup(name);
>> +	if (!prog->section_name) {
>> +		pr_warning("failed to alloc name for prog %s\n",
>> +			   name);
>> +		goto out;
>> +	}
>> +
>> +	prog->insns = malloc(size);
>> +	if (!prog->insns) {
>> +		pr_warning("failed to alloc insns for %s\n", name);
>> +		goto out;
>> +	}
>> +	prog->insns_cnt = size / sizeof(struct bpf_insn);
>> +	memcpy(prog->insns, data,
>> +	       prog->insns_cnt * sizeof(struct bpf_insn));
> Doesn't the data need to be swapped?
>
> Thanks,
> Namhyung
>

I'm not very sure, since they are instructions. Byte order of 
instructions and byte order of data
are not always same. Think about ARM. Therefore another choice is to 
swap them in kernel,
keep user-kernel interface clean.

Alexei Starovoitov, do you think we should use uniformed instruction 
byte order in both big and little
endian kernel on user-kernel interface, or let userspace feed swapped 
instructions to kernel if
endianess is not match?

Thank you.

>> +	prog->idx = idx;
>> +
>> +	return prog;
>> +out:
>> +	bpf_clear_program(prog);
>> +	return NULL;
>> +}
>> +
>>   static struct bpf_object *__bpf_obj_alloc(const char *path)
>>   {
>>   	struct bpf_object *obj;
>> @@ -380,6 +468,22 @@ static int bpf_obj_elf_collect(struct bpf_object *obj)
>>   				err = -EEXIST;
>>   			} else
>>   				obj->elf.symbols = data;
>> +		} else if ((sh.sh_type == SHT_PROGBITS) &&
>> +			   (sh.sh_flags & SHF_EXECINSTR) &&
>> +			   (data->d_size > 0)) {
>> +			struct bpf_program *prog;
>> +
>> +			prog = bpf_program_alloc(obj, data->d_buf,
>> +						 data->d_size, name,
>> +						 idx);
>> +			if (!prog) {
>> +				pr_warning("failed to alloc "
>> +					   "program %s (%s)", name,
>> +					   obj->path);
>> +				err = -ENOMEM;
>> +			} else
>> +				pr_debug("found program %s\n",
>> +					 prog->section_name);
>>   		}
>>   		if (err)
>>   			goto out;
>> @@ -446,5 +550,12 @@ void bpf_close_object(struct bpf_object *obj)
>>   		free(obj->maps_buf);
>>   	if (obj->config_str)
>>   		free(obj->config_str);
>> +	if (obj->programs) {
>> +		size_t i;
>> +
>> +		for (i = 0; i < obj->nr_programs; i++)
>> +			bpf_clear_program(&obj->programs[i]);
>> +		free(obj->programs);
>> +	}
>>   	free(obj);
>>   }
>> -- 
>> 1.8.3.4
>>