Re: [RFC PATCH net-next] bpf: fix potential percpu map overcopy to user.

[Daniel Borkmann <daniel@iogearbox.net>]

On 10/19/2016 07:31 AM, William Tu wrote:
>> ...
>>> -     if (copy_to_user(uvalue, value, value_size) != 0)
>>> +     if (copy_to_user(uvalue, value, min_t(u32, usize, value_size)) != 0)
>>>                goto free_value;
>>
>> I think such approach won't actually fix anything. User space
>> may lose some of the values and won't have any idea what was lost.
>> I think we need to fix sample code to avoid using sysconf(_SC_NPROCESSORS_CONF)
>> and use /sys/devices/system/cpu/possible instead.
>> I would argue that glibc should be fixed as well since relying on
>> ls -d /sys/devices/system/cpu/cpu[0-9]*|wc -l turned out to be incorrect.
>
> Thanks for the feedback. I think glibc is correct. The
> _SC_NPROCESSORS_CONF presents the number of processors
> configured/populated and is indeed "ls
> /sys/devices/system/cpu/cpu[0-9]*|wc -l". This means the actual number
> of CPUs installed on your system. On the other hand, the

In glibc __get_nprocs_conf() seems to try a number of things, first it
tries equivalent of /sys/devices/system/cpu/cpu[0-9]*|wc -l, if that fails,
depending on the config, it either tries to count cpus in /proc/cpuinfo,
or returns the _SC_NPROCESSORS_ONLN value instead. If /proc/cpuinfo has
some issue, it returns just 1 worst case. _SC_NPROCESSORS_ONLN will parse
/sys/devices/system/cpu/online, if that fails it looks into /proc/stat
for cpuX entries, and if also that fails for some reason, /proc/cpuinfo
is consulted (and returning 1 if unlikely all breaks down).

> num_possible_cpus() includes both the installed CPUs and the empty CPU
> socket/slot, in order to support CPU hotplug.

Correct.

> As a example, one of my dual socket motherboard with 1 CPU installed has
> # /sys/devices/system/cpu/possible
> 0-239
> # /sys/devices/system/cpu/cpu[0-9]*|wc -l
> 12
> Note that these 12 cpus could be online/offline by
> # echo 1/0 > /sys/devices/system/cpu/cpuX/online
> Even if it is offline, the entry is still there.
>
> Thinking about another solution, maybe we should use
> "num_present_cpus()" which means the configured/populated CPUs and the
> value is the same as sysconf(_SC_NPROCESSORS_CONF). Consider:
> 1) cpuX is online/offline: the num_present_cpus() remains the same.
> 2) new cpu is hotplug into the empty socket: the num_present_cpus()
> gets updates, and also the sysconf(_SC_NPROCESSORS_CONF).
>
> +++ b/kernel/bpf/syscall.c
> @@ -297,7 +297,7 @@ static int map_lookup_elem(union bpf_attr *attr)
>
>          if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
>              map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
> -               value_size = round_up(map->value_size, 8) * num_possible_cpus();
> +               value_size = round_up(map->value_size, 8) * num_present_cpus();
>          else
>                  value_size = map->value_size;

But as you say in 2) that also has a chance of being racy on CPU hotplug
compared to num_possible_cpus() which is fixed at boot time.

Documentation/cputopology.txt +106 says /sys/devices/system/cpu/possible
outputs cpu_possible_mask. That is the same as in num_possible_cpus(), so
first step would be to fix the buggy example code, imho.

What perhaps could be done in a second step to reduce overhead is an option
for bpf(2) to pass in a cpu mask similarly as for sched_{get,set}affinity()
syscalls, where user space can construct a mask via CPU_SET(3). For the
syscall time, kernel would lock hot plugging via get_online_cpus() and
put_online_cpus(), it would check whether passed CPUs are online to query
and if so then it would copy the values into the user provided buffer. I'd
think this might be useful in a number of ways anyway.

Thanks,
Daniel