On 07/29/2016 10:03 PM, William Tu wrote:
Hi Daniel and Alexei,
Thanks for the reply. My apology for too brief description. In short,
in my environment, running samples/bpf/test_map always segfault under
percpu array/hash map operations. I think it's due to stack
corruption.
I'm not using ARM. It's x86 in a VM with 2 vcpu. By printk() in kernel, I got
num_possible_cpu == 64
num_online_cpu == 2 == sysconf(_SC_NPROCESSORS_CONF)
Ok, thanks for the data!
So at samples/bpf/test_maps.c, test_percpu_arraymap_sanity(),
we define:
long values[nr_cpus]; //nr_cpus=2
... // create map and update map ...
/* check that key=0 is also found and zero initialized */
assert(bpf_lookup_elem(map_fd, &key, values) == 0 &&
values[0] == 0 && values[nr_cpus - 1] == 0);
Here we enter the bpf syscall, calls into kernel "map_lookup_elem()"
and we calculate:
value_size = round_up(map->value_size, 8) * num_possible_cpus();
// which in my case 8 * 64 = 512
...
// then copy to user, which writes 512B to the "values[nr_cpus]" on stack
if (copy_to_user(uvalue, value, value_size) != 0)
And I think this 512B write to userspace corrupts the userspace stack
and causes a coredump. After bpf_lookup_elem() calls, gdb shows
'values' points to memory address 0x0.
To fix it, I could either
1). declare values array based on num_possible_cpu in test_map.c,
long values[64];
or 2) in kernel, only copying 8*2 = 16 byte from kernel to user.
But I think the patch of using num_online_cpus() would also not be correct
in the sense that f.e. your application could alloc an array at time X
where map lookup at time Y would not fit to the expectations anymore due
to CPU hotplugging (since apparently _SC_NPROCESSORS_CONF maps to online
CPUs in some cases). So also there you could potentially corrupt your
application or mem allocator in user space, or not all your valid data
might get copied, hmm.
Regards,
William
On Fri, Jul 29, 2016 at 12:54 AM, Daniel Borkmann <dan...@iogearbox.net> wrote:
On 07/29/2016 08:47 AM, Alexei Starovoitov wrote:
On Thu, Jul 28, 2016 at 05:42:21PM -0700, William Tu wrote:
The total size of value copy_to_user() writes to userspace should
be the (current number of cpu) * (value size), instead of
num_possible_cpus() * (value size). Found by samples/bpf/test_maps.c,
which always copies 512 byte to userspace, crashing the userspace
program stack.
hmm. I'm missing something. The sample code assumes no cpu hutplug,
so sysconf(_SC_NPROCESSORS_CONF) == num_possible_cpu == num_online_cpu,
unless there is crazy INIT_ALL_POSSIBLE config option is used.
Are you using ARM by chance? What is the count that you get in
user space and from kernel side?
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/054177.html
