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

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

[Jia-Ju Bai]

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).


Best wishes,
Jia-Ju Bai

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

[Daniel Borkmann]

On 08/10/2018 04:07 PM, Jia-Ju Bai wrote:
> 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).

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,
Daniel

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

[Jia-Ju Bai]

On 2018/8/10 22:22, Daniel Borkmann wrote:
> On 08/10/2018 04:07 PM, Jia-Ju Bai wrote:
>> 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).
> 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?


Best wishes,
Jia-Ju Bai

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

[Y Song]

On Fri, Aug 10, 2018 at 6:57 PM, Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>
>
> On 2018/8/10 22:22, Daniel Borkmann wrote:
>>
>> On 08/10/2018 04:07 PM, Jia-Ju Bai wrote:
>>>
>>> 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?

>>
>> 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?

>
>
> Best wishes,
> Jia-Ju Bai

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

[Jia-Ju Bai]

On 2018/8/11 13:01, Y Song wrote:
> On Fri, Aug 10, 2018 at 6:57 PM, Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>>
>> On 2018/8/10 22:22, Daniel Borkmann wrote:
>>> On 08/10/2018 04:07 PM, Jia-Ju Bai wrote:
>>>> 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?
>
>>> 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