From: Jiri Olsa jolsa@kernel.org
[ Upstream commit 4121d4481b72501aa4d22680be4ea1096d69d133 ]
Hao Sun reported crash in dispatcher image [1].
Currently we don't have any sync between bpf_dispatcher_update and bpf_dispatcher_xdp_func, so following race is possible:
cpu 0: cpu 1:
bpf_prog_run_xdp ... bpf_dispatcher_xdp_func in image at offset 0x0
bpf_dispatcher_update update image at offset 0x800 bpf_dispatcher_update update image at offset 0x0
in image at offset 0x0 -> crash
Fixing this by synchronizing dispatcher image update (which is done in bpf_dispatcher_update function) with bpf_dispatcher_xdp_func that reads and execute the dispatcher image.
Calling synchronize_rcu after updating and installing new image ensures that readers leave old image before it's changed in the next dispatcher update. The update itself is locked with dispatcher's mutex.
The bpf_prog_run_xdp is called under local_bh_disable and synchronize_rcu will wait for it to leave [2].
[1] https://lore.kernel.org/bpf/Y5SFho7ZYXr9ifRn@krava/T/#m00c29ece654bc9f332a17... [2] https://lore.kernel.org/bpf/0B62D35A-E695-4B7A-A0D4-774767544C1A@gmail.com/T...
Reported-by: Hao Sun sunhao.th@gmail.com Signed-off-by: Jiri Olsa jolsa@kernel.org Acked-by: Yonghong Song yhs@fb.com Acked-by: Paul E. McKenney paulmck@kernel.org Link: https://lore.kernel.org/r/20221214123542.1389719-1-jolsa@kernel.org Signed-off-by: Martin KaFai Lau martin.lau@kernel.org (cherry picked from commit 4121d4481b72501aa4d22680be4ea1096d69d133) Signed-off-by: Sergio González Collado sergio.collado@gmail.com Reported-by: syzbot+08ba1e474d350b613604@syzkaller.appspotmail.com --- kernel/bpf/dispatcher.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/kernel/bpf/dispatcher.c b/kernel/bpf/dispatcher.c index c19719f48ce0..fa3e9225aedc 100644 --- a/kernel/bpf/dispatcher.c +++ b/kernel/bpf/dispatcher.c @@ -125,6 +125,11 @@ static void bpf_dispatcher_update(struct bpf_dispatcher *d, int prev_num_progs)
__BPF_DISPATCHER_UPDATE(d, new ?: (void *)&bpf_dispatcher_nop_func);
+ /* Make sure all the callers executing the previous/old half of the + * image leave it, so following update call can modify it safely. + */ + synchronize_rcu(); + if (new) d->image_off = noff; }