On Tue, Feb 9, 2021 at 10:00 PM Martin KaFai Lau <kafai@xxxxxx> wrote: > > 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+3536db46dfa58c573458@xxxxxxxxxxxxxxxxxxxxxxxxx > > Reported-by: syzbot+516acdb03d3e27d91bcd@xxxxxxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Marco Elver <elver@xxxxxxxxxx> > > --- > > 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 <kafai@xxxxxx> Added Fixes: 3a08c2fd7634 ("bpf: LRU List") and applied to bpf-next, thanks.
