On Fri, Jul 29, 2016 at 5:19 PM, Daniel Borkmann [off-list ref] wrote:

On 07/29/2016 10:03 PM, William Tu wrote:

quoted

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!

quoted

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.

Yes, you're right. CPU hotplugging might cause the same issue.

Since percpu array adds variable length of data passing between kernel
and userspace, I wonder if we should add a 'value_len' field in 'union
bpf_attr' so kernel knows how much data to copy to user?

Regards,
William