On Thu, Nov 05, 2020 at 07:34:55PM +0000, Song Liu wrote: > Hi, > > We are trying to fix some sparse warning in kernel/bpf/hashtab.c (in bpf-next tree). > The sparse I use was v0.6.3-76-gf680124b. > > These new warnings are introduced by [1]. Before [1], hashtab.c got > > [...] > kernel/bpf/hashtab.c:2089:19: error: subtraction of functions? Share your drugs > kernel/bpf/hashtab.c:1421:9: warning: context imbalance in '__htab_map_lookup_and_delete_batch' - different lock contexts for basic block > kernel/bpf/hashtab.c: note: in included file (through include/linux/workqueue.h, include/linux/bpf.h): > ./include/linux/rcupdate.h:693:9: warning: context imbalance in 'bpf_hash_map_seq_find_next' - unexpected unlock > ./include/linux/rcupdate.h:693:9: warning: context imbalance in 'bpf_hash_map_seq_stop' - unexpected unlock > > With [1], we got a few more warnings: > > [...] > kernel/bpf/hashtab.c:2141:19: error: subtraction of functions? Share your drugs > kernel/bpf/hashtab.c:1292:27: warning: context imbalance in 'htab_map_delete_elem' - unexpected unlock > kernel/bpf/hashtab.c:1325:27: warning: context imbalance in 'htab_lru_map_delete_elem' - unexpected unlock > kernel/bpf/hashtab.c: note: in included file (through include/linux/workqueue.h, include/linux/bpf.h): > ./include/linux/rcupdate.h:693:9: warning: context imbalance in '__htab_map_lookup_and_delete_batch' - unexpected unlock > ./include/linux/rcupdate.h:693:9: warning: context imbalance in 'bpf_hash_map_seq_find_next' - unexpected unlock > ./include/linux/rcupdate.h:693:9: warning: context imbalance in 'bpf_hash_map_seq_stop' - unexpected unlock > > So htab_map_delete_elem() and htab_lru_map_delete_elem() got new warnings, and > __htab_map_lookup_and_delete_batch() got a slightly different warning. > > After trying different annotations, including the attached foo.diff by Daniel, > we found the simplest fix was something like: Annotations can't fix anything here. The patch [1] changes htab_lock_bucket(), into conditionally taking a lock while before it took it unconditionally. So, each point where its value is tested now becomes the confluence of 2 distinct paths: one with the lock taken and one with the lock not taken. This is what is meant by 'context imbalance'. It should be noted that this 'context imbalance' doesn't mean things like 'this section of code can be executed without the lock held'. In the present case, if Sparse would be smarter (move around condition testing) it would be able to see something like 'OK, there is an imbalance but this has no impact on the lock being held when needed'. But, in the general case, it's impossible to do and so Sparse doesn't even try. -- Luc Van Oostenryck
