| Message ID | 20210209112701.3341724-1-elver@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6df8fb83301d68ea0a0c0e1cbcc790fcc333ed12 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf_lru_list: Read double-checked variable once without lock | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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@syzkaller.appspotmail.com > Reported-by: syzbot+516acdb03d3e27d91bcd@syzkaller.appspotmail.com > Signed-off-by: Marco Elver <elver@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 <kafai@fb.com>
On Tue, Feb 9, 2021 at 10:00 PM Martin KaFai Lau <kafai@fb.com> 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@syzkaller.appspotmail.com > > Reported-by: syzbot+516acdb03d3e27d91bcd@syzkaller.appspotmail.com > > Signed-off-by: Marco Elver <elver@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 <kafai@fb.com> Added Fixes: 3a08c2fd7634 ("bpf: LRU List") and applied to bpf-next, thanks.
Hello: This patch was applied to bpf/bpf-next.git (refs/heads/master): On Tue, 9 Feb 2021 12:27:01 +0100 you 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 > ... > > [...] Here is the summary with links: - bpf_lru_list: Read double-checked variable once without lock https://git.kernel.org/bpf/bpf-next/c/6df8fb83301d You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
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) { struct bpf_lru_locallist *loc_l; loc_l = per_cpu_ptr(lru->common_lru.local_list, node->cpu);
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@syzkaller.appspotmail.com Reported-by: syzbot+516acdb03d3e27d91bcd@syzkaller.appspotmail.com Signed-off-by: Marco Elver <elver@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(-)