Re: [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem()

On 2018/8/11 13:01, Y Song wrote:

On Fri, Aug 10, 2018 at 6:57 PM, Jia-Ju Bai [off-list ref] wrote:

quoted

On 2018/8/10 22:22, Daniel Borkmann wrote:

quoted

On 08/10/2018 04:07 PM, Jia-Ju Bai wrote:

quoted

The kernel may sleep with holding a rcu read lock.

The function call paths (from bottom to top) in Linux-4.16 are:

[FUNC] kmalloc(GFP_KERNEL)
kernel/kthread.c, 283: kmalloc in __kthread_create_on_node
kernel/kthread.c, 365: __kthread_create_on_node in kthread_create_on_node
kernel/bpf/cpumap.c, 368: kthread_create_on_node in __cpu_map_entry_alloc
kernel/bpf/cpumap.c, 490: __cpu_map_entry_alloc in cpu_map_update_elem
kernel/bpf/syscall.c, 724: [FUNC_PTR]cpu_map_update_elem in
map_update_elem
kernel/bpf/syscall.c, 723: rcu_read_lock in map_update_elem

Note that [FUNC_PTR] means a function pointer call is used.

I do not find a good way to fix it, so I only report.
This is found by my static analysis tool (DSAC).

Maybe your static analysis tool have false positives in this case?

quoted

quoted

Thanks for the report Jia-Ju! In the map_update_elem() from syscall
path there's a check map->map_type == BPF_MAP_TYPE_CPUMAP, where we
call the cpumap's map->ops->map_update_elem() while /not/ being under
rcu_read_lock() as in other cases, so looks okay to me. Could you point
out the case for being under rcu_read_lock() more specifically which
the tool found?

Thanks for your reply :)
My tool cannot accurately track the case of map->map_type at present...

According to my code review, there is a indeed check on line 697 in
Linux-4.16:
     else if (map->map_type == BPF_MAP_TYPE_CPUMAP) {
         err = map->ops->map_update_elem(map, key, value, attr->flags);
         goto out;
     }
But there is a call to map->ops->map_update_elem() that is under
rcu_read_lock on line 724:
         rcu_read_lock();
         err = map->ops->map_update_elem(map, key, value, attr->flags);
         rcu_read_unlock();

So I think if map->map_type is not equal to BPF_MAP_TYPE_CPUMAP,
map->ops->map_update_elem() can still be called under rcu_read_lock, is it
right?

map->ops->map_update_elem() can be called under rcu_read_lock(), but
since it is not type cpumap, the function should not be cpu_map_update_elem().
Could you double check your static analysis tool?

Thanks for your reply :)

I have checked the code again, and find that my report is not correct here.
It is because that my tool does not handle the context of function 
pointer assignment.


Best wishes,
Jia-Ju Bai