* [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem() @ 2018-08-10 14:07 Jia-Ju Bai 2018-08-10 14:22 ` Daniel Borkmann 0 siblings, 1 reply; 5+ messages in thread From: Jia-Ju Bai @ 2018-08-10 14:07 UTC (permalink / raw) To: ast, daniel; +Cc: netdev, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem() 2018-08-10 14:07 [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem() Jia-Ju Bai @ 2018-08-10 14:22 ` Daniel Borkmann 2018-08-11 1:57 ` Jia-Ju Bai 0 siblings, 1 reply; 5+ messages in thread From: Daniel Borkmann @ 2018-08-10 14:22 UTC (permalink / raw) To: Jia-Ju Bai, ast; +Cc: netdev, Linux Kernel Mailing List, brouer 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem() 2018-08-10 14:22 ` Daniel Borkmann @ 2018-08-11 1:57 ` Jia-Ju Bai 2018-08-11 5:01 ` Y Song 0 siblings, 1 reply; 5+ messages in thread From: Jia-Ju Bai @ 2018-08-11 1:57 UTC (permalink / raw) To: Daniel Borkmann, ast; +Cc: netdev, Linux Kernel Mailing List, brouer 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem() 2018-08-11 1:57 ` Jia-Ju Bai @ 2018-08-11 5:01 ` Y Song 2018-08-13 2:48 ` Jia-Ju Bai 0 siblings, 1 reply; 5+ messages in thread From: Y Song @ 2018-08-11 5:01 UTC (permalink / raw) To: Jia-Ju Bai Cc: Daniel Borkmann, Alexei Starovoitov, netdev, Linux Kernel Mailing List, Jesper Dangaard Brouer 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem() 2018-08-11 5:01 ` Y Song @ 2018-08-13 2:48 ` Jia-Ju Bai 0 siblings, 0 replies; 5+ messages in thread From: Jia-Ju Bai @ 2018-08-13 2:48 UTC (permalink / raw) To: Y Song Cc: Daniel Borkmann, Alexei Starovoitov, netdev, Linux Kernel Mailing List, Jesper Dangaard Brouer 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-13 2:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-10 14:07 [BUG] bpf: syscall: a possible sleep-in-atomic-context bug in map_update_elem() Jia-Ju Bai 2018-08-10 14:22 ` Daniel Borkmann 2018-08-11 1:57 ` Jia-Ju Bai 2018-08-11 5:01 ` Y Song 2018-08-13 2:48 ` Jia-Ju Bai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).