bpf: devmap: Pass lockdep expression to RCU lists
Subsystems (4)
PATCH 1/1 bpf: devmap: Pass lockdep expression to RCU lists
head is traversed using hlist_for_each_entry_rcu outside an RCU read-side critical section but under the protection of dtab->index_lock.
Hence, add corresponding lockdep expression to silence false-positive lockdep warnings, and harden RCU lists.
--- kernel/bpf/devmap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/bpf/devmap.c +2 -1
@@ -293,7 +293,8 @@ struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)struct hlist_head *head =dev_map_index_hash (dtab, key);struct bpf_dtab_netdev *dev; -hlist_for_each_entry_rcu (dev, head, index_hlist) +hlist_for_each_entry_rcu (dev, head, index_hlist, +lockdep_is_held (&dtab->index_lock))if (dev->idx == key)return dev;
Signature · 1 line
2.24.1
Quoted 3 lines · On Thu, 23 Jan 2020 17:34:38 +0530 Amol Grover <[email protected]> wrote:
head is traversed using
hlist_for_each_entry_rcuoutside an RCU read-side critical section but under the protection ofdtab->index_lock.
We do hold the lock in update and delete cases, but not in the lookup cases. Is it then still okay to add the lockdep_is_held() annotation?
If it is then it looks fine to me:
Quoted 22 lines
Hence, add corresponding lockdep expression to silence false-positive lockdep warnings, and harden RCU lists.
Signed-off-by Amol Grover <[email protected]>--- kernel/bpf/devmap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/devmap.c +2
-1@@ -293,7 +293,8 @@ struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)struct hlist_head *head =dev_map_index_hash (dtab, key);struct bpf_dtab_netdev *dev; -hlist_for_each_entry_rcu (dev, head, index_hlist) +hlist_for_each_entry_rcu (dev, head, index_hlist, +lockdep_is_held (&dtab->index_lock))if (dev->idx == key)return dev;
Signature · 4 lines
Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer
Quoted 9 lines · Jesper Dangaard Brouer
Quoted 3 lines · On Thu, 23 Jan 2020 17:34:38 +0530 Amol Grover <[email protected]> wrote:
head is traversed using
hlist_for_each_entry_rcuoutside an RCU read-side critical section but under the protection ofdtab->index_lock.We do hold the lock in update and delete cases, but not in the lookup cases. Is it then still okay to add the
lockdep_is_held()annotation?
I concluded 'yes' from the comment on hlist_for_each_entry_rcu():
The lockdep condition gets passed to this:
#define __list_check_rcu(dummy, cond, extra...) \
({ \
check_arg_count_one(extra); \
RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
"RCU-list traversed in non-reader section!"); \
})so that seems fine :)
-Toke
Quoted 24 lines · Toke Høiland-Jørgensen
Quoted 9 lines · Jesper Dangaard Brouer
Quoted 3 lines · On Thu, 23 Jan 2020 17:34:38 +0530 Amol Grover <[email protected]> wrote:
head is traversed using
hlist_for_each_entry_rcuoutside an RCU read-side critical section but under the protection ofdtab->index_lock.We do hold the lock in update and delete cases, but not in the lookup cases. Is it then still okay to add the
lockdep_is_held()annotation?I concluded 'yes' from the comment on
hlist_for_each_entry_rcu():The lockdep condition gets passed to this:
#define __list_check_rcu(dummy, cond, extra...) \ ({ \ check_arg_count_one(extra); \ RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \ "RCU-list traversed in non-reader section!"); \ })
Quoted 2 lines
so that seems fine :)
Yes, adding a lockdep expression will be okay. This is because an implicit check is done to check if list_for_each_entry_rcu() is traversed under RCU read-side critical section. In case the traversal is outside RCU read-side critical section, the lockdep expression makes sure the traversal is done under the mentioned lock.
Thanks
Amol
Quoted 2 lines
-Toke
Quoted 8 lines · Amol Grover
head is traversed using
hlist_for_each_entry_rcuoutside an RCU read-side critical section but under the protection ofdtab->index_lock.Hence, add corresponding lockdep expression to silence false-positive lockdep warnings, and harden RCU lists.
Signed-off-by Amol Grover <[email protected]>
Could you please add an appropriate Fixes: tag?
Otherwise:
Daniel Borkmann
maintainerQuoted 6 lines · Toke Høiland-Jørgensen
Quoted 4 lines · Amol Grover
head is traversed using
hlist_for_each_entry_rcuoutside an RCU read-side critical section but under the protection ofdtab->index_lock.
Quoted 6 lines
Quoted 4 lines
Hence, add corresponding lockdep expression to silence false-positive lockdep warnings, and harden RCU lists.
Signed-off-by Amol Grover <[email protected]>Could you please add an appropriate Fixes: tag?
+1, please reply with Fixes: tag (no need to resend).
Quoted 7 lines · Amol Grover
head is traversed using
hlist_for_each_entry_rcuoutside an RCU read-side critical section but under the protection ofdtab->index_lock.Hence, add corresponding lockdep expression to silence false-positive lockdep warnings, and harden RCU lists.
Quoted 22 lines
Signed-off-by Amol Grover <[email protected]>--- kernel/bpf/devmap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/bpf/devmap.c +2
-1@@ -293,7 +293,8 @@ struct bpf_dtab_netdev *__dev_map_hash_lookup_elem(struct bpf_map *map, u32 key)struct hlist_head *head =dev_map_index_hash (dtab, key);struct bpf_dtab_netdev *dev; -hlist_for_each_entry_rcu (dev, head, index_hlist) +hlist_for_each_entry_rcu (dev, head, index_hlist, +lockdep_is_held (&dtab->index_lock))if (dev->idx == key)return dev;Signature · 1 line
2.24.1
Quoted 10 lines · Amol Grover
Quoted 7 lines · Amol Grover
head is traversed using
hlist_for_each_entry_rcuoutside an RCU read-side critical section but under the protection ofdtab->index_lock.Hence, add corresponding lockdep expression to silence false-positive lockdep warnings, and harden RCU lists.
Fixes 6f9d451ab1a3 ("xdp: Add devmap_hash map type for looking up devices by hashed index")Quoted 1 line
Signed-off-by Amol Grover <[email protected]>
Applied, thanks!