[PATCH bpf] bpf: Synchronize dispatcher update with bpf_dispatcher_xdp_func

[PATCH bpf] bpf: Synchronize dispatcher update with bpf_dispatcher_xdp_func

[Jiri Olsa]

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/#m00c29ece654bc9f332a17df493bbca33e702896c
[2] https://lore.kernel.org/bpf/0B62D35A-E695-4B7A-A0D4-774767544C1A@gmail.com/T/#mff43e2c003ae99f4a38f353c7969be4c7162e877

Reported-by: Hao Sun <sunhao.th@gmail.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 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;
 }
-- 
2.38.1

Re: [PATCH bpf] bpf: Synchronize dispatcher update with bpf_dispatcher_xdp_func

[Yonghong Song]

On 12/14/22 4:35 AM, Jiri Olsa wrote:
> 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/#m00c29ece654bc9f332a17df493bbca33e702896c
> [2] https://lore.kernel.org/bpf/0B62D35A-E695-4B7A-A0D4-774767544C1A@gmail.com/T/#mff43e2c003ae99f4a38f353c7969be4c7162e877
> 
> Reported-by: Hao Sun <sunhao.th@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH bpf] bpf: Synchronize dispatcher update with bpf_dispatcher_xdp_func

[Paul E. McKenney]

On Wed, Dec 14, 2022 at 01:35:42PM +0100, Jiri Olsa wrote:
> 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/#m00c29ece654bc9f332a17df493bbca33e702896c
> [2] https://lore.kernel.org/bpf/0B62D35A-E695-4B7A-A0D4-774767544C1A@gmail.com/T/#mff43e2c003ae99f4a38f353c7969be4c7162e877
> 
> Reported-by: Hao Sun <sunhao.th@gmail.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

From an RCU viewpoint:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  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;
>  }
> -- 
> 2.38.1
> 

Re: [PATCH bpf] bpf: Synchronize dispatcher update with bpf_dispatcher_xdp_func

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Wed, 14 Dec 2022 13:35:42 +0100 you wrote:
> 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:
> 
> [...]

Here is the summary with links:
  - [bpf] bpf: Synchronize dispatcher update with bpf_dispatcher_xdp_func
    https://git.kernel.org/bpf/bpf/c/4121d4481b72

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html