On Tue, Feb 09, 2021 at 12:27:01PM +0100, Marco Elver wrote: > For double-checked locking in bpf_common_lru_push_free(), node->type is > read outside the critical section and then re-checked under the lock. > However, concurrent writes to node->type result in data races. > > For example, the following concurrent access was observed by KCSAN: > > write to 0xffff88801521bc22 of 1 bytes by task 10038 on cpu 1: > __bpf_lru_node_move_in kernel/bpf/bpf_lru_list.c:91 > __local_list_flush kernel/bpf/bpf_lru_list.c:298 > ... > read to 0xffff88801521bc22 of 1 bytes by task 10043 on cpu 0: > bpf_common_lru_push_free kernel/bpf/bpf_lru_list.c:507 > bpf_lru_push_free kernel/bpf/bpf_lru_list.c:555 > ... > > Fix the data races where node->type is read outside the critical section > (for double-checked locking) by marking the access with READ_ONCE() as > well as ensuring the variable is only accessed once. > > Reported-by: syzbot+3536db46dfa58c573...@syzkaller.appspotmail.com > Reported-by: syzbot+516acdb03d3e27d91...@syzkaller.appspotmail.com > Signed-off-by: Marco Elver <el...@google.com> > --- > Detailed reports: > > https://groups.google.com/g/syzkaller-upstream-moderation/c/PwsoQ7bfi8k/m/NH9Ni2WxAQAJ > > > https://groups.google.com/g/syzkaller-upstream-moderation/c/-fXQO9ehxSM/m/RmQEcI2oAQAJ > > --- > kernel/bpf/bpf_lru_list.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/kernel/bpf/bpf_lru_list.c b/kernel/bpf/bpf_lru_list.c > index 1b6b9349cb85..d99e89f113c4 100644 > --- a/kernel/bpf/bpf_lru_list.c > +++ b/kernel/bpf/bpf_lru_list.c > @@ -502,13 +502,14 @@ struct bpf_lru_node *bpf_lru_pop_free(struct bpf_lru > *lru, u32 hash) > static void bpf_common_lru_push_free(struct bpf_lru *lru, > struct bpf_lru_node *node) > { > + u8 node_type = READ_ONCE(node->type); > unsigned long flags; > > - if (WARN_ON_ONCE(node->type == BPF_LRU_LIST_T_FREE) || > - WARN_ON_ONCE(node->type == BPF_LRU_LOCAL_LIST_T_FREE)) > + if (WARN_ON_ONCE(node_type == BPF_LRU_LIST_T_FREE) || > + WARN_ON_ONCE(node_type == BPF_LRU_LOCAL_LIST_T_FREE)) > return; > > - if (node->type == BPF_LRU_LOCAL_LIST_T_PENDING) { > + if (node_type == BPF_LRU_LOCAL_LIST_T_PENDING) { I think this can be bpf-next.
Acked-by: Martin KaFai Lau <ka...@fb.com>
