[PATCHv2 bpf 0/2] bpf: Fix prog_array_map_poke_run map poke update

[PATCHv2 bpf 0/2] bpf: Fix prog_array_map_poke_run map poke update

[Jiri Olsa]

hi,
this patchset fixes the issue reported in [0].

For the actual fix in patch 2 I'm changing bpf_arch_text_poke to allow to skip
ip address check in patch 1. I considered adding separate function for that,
but because each arch implementation is bit different, adding extra arg seemed
like better option.

v2 changes:
  - make it work for other archs

thanks,
jirka


[0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
---
Jiri Olsa (2):
      bpf: Add checkip argument to bpf_arch_text_poke
      bpf, x64: Fix prog_array_map_poke_run map poke update

 arch/arm64/net/bpf_jit_comp.c   |  3 ++-
 arch/riscv/net/bpf_jit_comp64.c |  5 +++--
 arch/s390/net/bpf_jit_comp.c    |  3 ++-
 arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
 include/linux/bpf.h             |  2 +-
 kernel/bpf/arraymap.c           | 31 +++++++++++--------------------
 kernel/bpf/core.c               |  2 +-
 kernel/bpf/trampoline.c         | 12 ++++++------
 8 files changed, 39 insertions(+), 43 deletions(-)

[PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Jiri Olsa]

We need to be able to skip ip address check for caller in following
changes. Adding checkip argument to allow that.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 arch/arm64/net/bpf_jit_comp.c   |  3 ++-
 arch/riscv/net/bpf_jit_comp64.c |  5 +++--
 arch/s390/net/bpf_jit_comp.c    |  3 ++-
 arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
 include/linux/bpf.h             |  2 +-
 kernel/bpf/arraymap.c           |  8 ++++----
 kernel/bpf/core.c               |  2 +-
 kernel/bpf/trampoline.c         | 12 ++++++------
 8 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index 7d4af64e3982..b52549d18730 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -2167,7 +2167,8 @@ static int gen_branch_or_nop(enum aarch64_insn_branch_type type, void *ip,
  * locations during the patching process, making the patching process easier.
  */
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
-		       void *old_addr, void *new_addr)
+		       void *old_addr, void *new_addr,
+		       bool checkip __maybe_unused)
 {
 	int ret;
 	u32 old_insn;
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index 8581693e62d3..cd1c9fa39a03 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -667,13 +667,14 @@ static int gen_jump_or_nops(void *target, void *ip, u32 *insns, bool is_call)
 }
 
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
-		       void *old_addr, void *new_addr)
+		       void *old_addr, void *new_addr, bool checkip)
 {
 	u32 old_insns[RV_FENTRY_NINSNS], new_insns[RV_FENTRY_NINSNS];
 	bool is_call = poke_type == BPF_MOD_CALL;
 	int ret;
 
-	if (!is_kernel_text((unsigned long)ip) &&
+	if (checkip &&
+	    !is_kernel_text((unsigned long)ip) &&
 	    !is_bpf_text_address((unsigned long)ip))
 		return -ENOTSUPP;
 
diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index bf06b7283f0c..7333a78a30e5 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -2146,7 +2146,8 @@ bool bpf_jit_supports_far_kfunc_call(void)
 }
 
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
-		       void *old_addr, void *new_addr)
+		       void *old_addr, void *new_addr,
+		       bool checkip __maybe_unused)
 {
 	struct {
 		u16 opc;
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 8c10d9abc239..163bb392c02e 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -435,19 +435,21 @@ static int __bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
 }
 
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
-		       void *old_addr, void *new_addr)
+		       void *old_addr, void *new_addr, bool checkip)
 {
-	if (!is_kernel_text((long)ip) &&
-	    !is_bpf_text_address((long)ip))
-		/* BPF poking in modules is not supported */
-		return -EINVAL;
+	if (checkip) {
+		if (!is_kernel_text((long)ip) &&
+		    !is_bpf_text_address((long)ip))
+			/* BPF poking in modules is not supported */
+			return -EINVAL;
 
-	/*
-	 * See emit_prologue(), for IBT builds the trampoline hook is preceded
-	 * with an ENDBR instruction.
-	 */
-	if (is_endbr(*(u32 *)ip))
-		ip += ENDBR_INSN_SIZE;
+		/*
+		 * See emit_prologue(), for IBT builds the trampoline hook is preceded
+		 * with an ENDBR instruction.
+		 */
+		if (is_endbr(*(u32 *)ip))
+			ip += ENDBR_INSN_SIZE;
+	}
 
 	return __bpf_arch_text_poke(ip, t, old_addr, new_addr);
 }
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6762dac3ef76..182544e12ef4 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3173,7 +3173,7 @@ enum bpf_text_poke_type {
 };
 
 int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
-		       void *addr1, void *addr2);
+		       void *addr1, void *addr2, bool checkip);
 
 void *bpf_arch_text_copy(void *dst, void *src, size_t len);
 int bpf_arch_text_invalidate(void *dst, size_t len);
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 2058e89b5ddd..7ba389f7212f 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -1075,20 +1075,20 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 			if (new) {
 				ret = bpf_arch_text_poke(poke->tailcall_target,
 							 BPF_MOD_JUMP,
-							 old_addr, new_addr);
+							 old_addr, new_addr, true);
 				BUG_ON(ret < 0 && ret != -EINVAL);
 				if (!old) {
 					ret = bpf_arch_text_poke(poke->tailcall_bypass,
 								 BPF_MOD_JUMP,
 								 poke->bypass_addr,
-								 NULL);
+								 NULL, true);
 					BUG_ON(ret < 0 && ret != -EINVAL);
 				}
 			} else {
 				ret = bpf_arch_text_poke(poke->tailcall_bypass,
 							 BPF_MOD_JUMP,
 							 old_bypass_addr,
-							 poke->bypass_addr);
+							 poke->bypass_addr, true);
 				BUG_ON(ret < 0 && ret != -EINVAL);
 				/* let other CPUs finish the execution of program
 				 * so that it will not possible to expose them
@@ -1098,7 +1098,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 					synchronize_rcu();
 				ret = bpf_arch_text_poke(poke->tailcall_target,
 							 BPF_MOD_JUMP,
-							 old_addr, NULL);
+							 old_addr, NULL, true);
 				BUG_ON(ret < 0 && ret != -EINVAL);
 			}
 		}
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index cd3afe57ece3..c7fdc68116f3 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2903,7 +2903,7 @@ int __weak skb_copy_bits(const struct sk_buff *skb, int offset, void *to,
 }
 
 int __weak bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
-			      void *addr1, void *addr2)
+			      void *addr1, void *addr2, bool checkip)
 {
 	return -ENOTSUPP;
 }
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index e97aeda3a86b..826f08f26e10 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -179,7 +179,7 @@ static int unregister_fentry(struct bpf_trampoline *tr, void *old_addr)
 	if (tr->func.ftrace_managed)
 		ret = unregister_ftrace_direct(tr->fops, (long)old_addr, false);
 	else
-		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL);
+		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, NULL, true);
 
 	return ret;
 }
@@ -196,7 +196,7 @@ static int modify_fentry(struct bpf_trampoline *tr, void *old_addr, void *new_ad
 		else
 			ret = modify_ftrace_direct_nolock(tr->fops, (long)new_addr);
 	} else {
-		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, new_addr);
+		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, old_addr, new_addr, true);
 	}
 	return ret;
 }
@@ -219,7 +219,7 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
 		ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 1);
 		ret = register_ftrace_direct(tr->fops, (long)new_addr);
 	} else {
-		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr);
+		ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr, true);
 	}
 
 	return ret;
@@ -331,7 +331,7 @@ static void bpf_tramp_image_put(struct bpf_tramp_image *im)
 	 */
 	if (im->ip_after_call) {
 		int err = bpf_arch_text_poke(im->ip_after_call, BPF_MOD_JUMP,
-					     NULL, im->ip_epilogue);
+					     NULL, im->ip_epilogue, true);
 		WARN_ON(err);
 		if (IS_ENABLED(CONFIG_PREEMPTION))
 			call_rcu_tasks(&im->rcu, __bpf_tramp_image_put_rcu_tasks);
@@ -533,7 +533,7 @@ static int __bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_tr
 			return -EBUSY;
 		tr->extension_prog = link->link.prog;
 		return bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP, NULL,
-					  link->link.prog->bpf_func);
+					  link->link.prog->bpf_func, true);
 	}
 	if (cnt >= BPF_MAX_TRAMP_LINKS)
 		return -E2BIG;
@@ -576,7 +576,7 @@ static int __bpf_trampoline_unlink_prog(struct bpf_tramp_link *link, struct bpf_
 	if (kind == BPF_TRAMP_REPLACE) {
 		WARN_ON_ONCE(!tr->extension_prog);
 		err = bpf_arch_text_poke(tr->func.addr, BPF_MOD_JUMP,
-					 tr->extension_prog->bpf_func, NULL);
+					 tr->extension_prog->bpf_func, NULL, true);
 		tr->extension_prog = NULL;
 		return err;
 	}
-- 
2.43.0

Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Stanislav Fomichev]

On 11/28, Jiri Olsa wrote:
> We need to be able to skip ip address check for caller in following
> changes. Adding checkip argument to allow that.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
>  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
>  arch/s390/net/bpf_jit_comp.c    |  3 ++-
>  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
>  include/linux/bpf.h             |  2 +-
>  kernel/bpf/arraymap.c           |  8 ++++----
>  kernel/bpf/core.c               |  2 +-
>  kernel/bpf/trampoline.c         | 12 ++++++------
>  8 files changed, 32 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 7d4af64e3982..b52549d18730 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -2167,7 +2167,8 @@ static int gen_branch_or_nop(enum aarch64_insn_branch_type type, void *ip,
>   * locations during the patching process, making the patching process easier.
>   */
>  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
> -		       void *old_addr, void *new_addr)
> +		       void *old_addr, void *new_addr,

[..]

> +		       bool checkip __maybe_unused)

Any idea why only riscv and x86 do this check?

Asking because maybe it makes sense to move this check into some
new generic bpf_text_poke and call it in the places where you currently
call checkip=true (and keep using bpf_arch_text_poke for checkip=false
case).

(don't see any issues with the current approach btw, just interested..)

Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Jiri Olsa]

On Tue, Nov 28, 2023 at 01:24:14PM -0800, Stanislav Fomichev wrote:
> On 11/28, Jiri Olsa wrote:
> > We need to be able to skip ip address check for caller in following
> > changes. Adding checkip argument to allow that.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> >  include/linux/bpf.h             |  2 +-
> >  kernel/bpf/arraymap.c           |  8 ++++----
> >  kernel/bpf/core.c               |  2 +-
> >  kernel/bpf/trampoline.c         | 12 ++++++------
> >  8 files changed, 32 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index 7d4af64e3982..b52549d18730 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -2167,7 +2167,8 @@ static int gen_branch_or_nop(enum aarch64_insn_branch_type type, void *ip,
> >   * locations during the patching process, making the patching process easier.
> >   */
> >  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
> > -		       void *old_addr, void *new_addr)
> > +		       void *old_addr, void *new_addr,
> 
> [..]
> 
> > +		       bool checkip __maybe_unused)
> 
> Any idea why only riscv and x86 do this check?

so arm does the check as well, but needs the data from the lookup
to patch things properly.. but IIUC it does not suffer the same
issue because it does not implement direct tail calls [1] which
is used only on x86

> 
> Asking because maybe it makes sense to move this check into some
> new generic bpf_text_poke and call it in the places where you currently
> call checkip=true (and keep using bpf_arch_text_poke for checkip=false
> case).
> 
> (don't see any issues with the current approach btw, just interested..)

I tried to add new function for that, but it did not look good for arm
because it needs to do the lookup anyway

hm maybe we could use new arch function that would cover the single
tail call 'text poke' update in prog_array_map_poke_run and would be
implemented only on x86 ... using __bpf_arch_text_poke directly

jirka


[1] 428d5df1fa4f bpf, x86: Emit patchable direct jump as tail call

Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Jiri Olsa]

On Wed, Nov 29, 2023 at 03:05:21PM +0100, Jiri Olsa wrote:
> On Tue, Nov 28, 2023 at 01:24:14PM -0800, Stanislav Fomichev wrote:
> > On 11/28, Jiri Olsa wrote:
> > > We need to be able to skip ip address check for caller in following
> > > changes. Adding checkip argument to allow that.
> > > 
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> > >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> > >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> > >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> > >  include/linux/bpf.h             |  2 +-
> > >  kernel/bpf/arraymap.c           |  8 ++++----
> > >  kernel/bpf/core.c               |  2 +-
> > >  kernel/bpf/trampoline.c         | 12 ++++++------
> > >  8 files changed, 32 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > > index 7d4af64e3982..b52549d18730 100644
> > > --- a/arch/arm64/net/bpf_jit_comp.c
> > > +++ b/arch/arm64/net/bpf_jit_comp.c
> > > @@ -2167,7 +2167,8 @@ static int gen_branch_or_nop(enum aarch64_insn_branch_type type, void *ip,
> > >   * locations during the patching process, making the patching process easier.
> > >   */
> > >  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
> > > -		       void *old_addr, void *new_addr)
> > > +		       void *old_addr, void *new_addr,
> > 
> > [..]
> > 
> > > +		       bool checkip __maybe_unused)
> > 
> > Any idea why only riscv and x86 do this check?
> 
> so arm does the check as well, but needs the data from the lookup
> to patch things properly.. but IIUC it does not suffer the same
> issue because it does not implement direct tail calls [1] which
> is used only on x86
> 
> > 
> > Asking because maybe it makes sense to move this check into some
> > new generic bpf_text_poke and call it in the places where you currently
> > call checkip=true (and keep using bpf_arch_text_poke for checkip=false
> > case).
> > 
> > (don't see any issues with the current approach btw, just interested..)
> 
> I tried to add new function for that, but it did not look good for arm
> because it needs to do the lookup anyway
> 
> hm maybe we could use new arch function that would cover the single
> tail call 'text poke' update in prog_array_map_poke_run and would be
> implemented only on x86 ... using __bpf_arch_text_poke directly

looks like below change would be enough, I'll test and send new version

jirka


---
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 8c10d9abc239..9c41c8c19eea 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -3025,3 +3025,44 @@ void arch_bpf_stack_walk(bool (*consume_fn)(void *cookie, u64 ip, u64 sp, u64 bp
 #endif
 	WARN(1, "verification of programs using bpf_throw should have failed\n");
 }
+
+void bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
+			       struct bpf_prog *new, struct bpf_prog *old)
+{
+	u8 *old_addr, *new_addr, *old_bypass_addr;
+	int ret;
+
+	old_bypass_addr = old ? NULL : poke->bypass_addr;
+	old_addr = old ? (u8 *)old->bpf_func + poke->adj_off : NULL;
+	new_addr = new ? (u8 *)new->bpf_func + poke->adj_off : NULL;
+
+	if (new) {
+		ret = __bpf_arch_text_poke(poke->tailcall_target,
+					   BPF_MOD_JUMP,
+					   old_addr, new_addr);
+		BUG_ON(ret < 0);
+		if (!old) {
+			ret = __bpf_arch_text_poke(poke->tailcall_bypass,
+						   BPF_MOD_JUMP,
+						   poke->bypass_addr,
+						   NULL);
+			BUG_ON(ret < 0);
+		}
+	} else {
+		ret = __bpf_arch_text_poke(poke->tailcall_bypass,
+					   BPF_MOD_JUMP,
+					   old_bypass_addr,
+					   poke->bypass_addr);
+		BUG_ON(ret < 0);
+		/* let other CPUs finish the execution of program
+		 * so that it will not possible to expose them
+		 * to invalid nop, stack unwind, nop state
+		 */
+		if (!ret)
+			synchronize_rcu();
+		ret = __bpf_arch_text_poke(poke->tailcall_target,
+					   BPF_MOD_JUMP,
+					   old_addr, NULL);
+		BUG_ON(ret < 0);
+	}
+}
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 2058e89b5ddd..73b6e6e3a8fd 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -1012,11 +1012,16 @@ static void prog_array_map_poke_untrack(struct bpf_map *map,
 	mutex_unlock(&aux->poke_mutex);
 }
 
+void __weak bpf_arch_poke_desc_update(struct bpf_jit_poke_descriptor *poke,
+				      struct bpf_prog *new, struct bpf_prog *old)
+{
+	WARN_ON_ONCE(1);
+}
+
 static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 				    struct bpf_prog *old,
 				    struct bpf_prog *new)
 {
-	u8 *old_addr, *new_addr, *old_bypass_addr;
 	struct prog_poke_elem *elem;
 	struct bpf_array_aux *aux;
 
@@ -1025,7 +1030,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 
 	list_for_each_entry(elem, &aux->poke_progs, list) {
 		struct bpf_jit_poke_descriptor *poke;
-		int i, ret;
+		int i;
 
 		for (i = 0; i < elem->aux->size_poke_tab; i++) {
 			poke = &elem->aux->poke_tab[i];
@@ -1068,39 +1073,7 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 			    poke->tail_call.key != key)
 				continue;
 
-			old_bypass_addr = old ? NULL : poke->bypass_addr;
-			old_addr = old ? (u8 *)old->bpf_func + poke->adj_off : NULL;
-			new_addr = new ? (u8 *)new->bpf_func + poke->adj_off : NULL;
-
-			if (new) {
-				ret = bpf_arch_text_poke(poke->tailcall_target,
-							 BPF_MOD_JUMP,
-							 old_addr, new_addr);
-				BUG_ON(ret < 0 && ret != -EINVAL);
-				if (!old) {
-					ret = bpf_arch_text_poke(poke->tailcall_bypass,
-								 BPF_MOD_JUMP,
-								 poke->bypass_addr,
-								 NULL);
-					BUG_ON(ret < 0 && ret != -EINVAL);
-				}
-			} else {
-				ret = bpf_arch_text_poke(poke->tailcall_bypass,
-							 BPF_MOD_JUMP,
-							 old_bypass_addr,
-							 poke->bypass_addr);
-				BUG_ON(ret < 0 && ret != -EINVAL);
-				/* let other CPUs finish the execution of program
-				 * so that it will not possible to expose them
-				 * to invalid nop, stack unwind, nop state
-				 */
-				if (!ret)
-					synchronize_rcu();
-				ret = bpf_arch_text_poke(poke->tailcall_target,
-							 BPF_MOD_JUMP,
-							 old_addr, NULL);
-				BUG_ON(ret < 0 && ret != -EINVAL);
-			}
+			bpf_arch_poke_desc_update(poke, new, old);
 		}
 	}
 }

Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Stanislav Fomichev]

On Wed, Nov 29, 2023 at 6:55 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Nov 29, 2023 at 03:05:21PM +0100, Jiri Olsa wrote:
> > On Tue, Nov 28, 2023 at 01:24:14PM -0800, Stanislav Fomichev wrote:
> > > On 11/28, Jiri Olsa wrote:
> > > > We need to be able to skip ip address check for caller in following
> > > > changes. Adding checkip argument to allow that.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> > > >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> > > >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> > > >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> > > >  include/linux/bpf.h             |  2 +-
> > > >  kernel/bpf/arraymap.c           |  8 ++++----
> > > >  kernel/bpf/core.c               |  2 +-
> > > >  kernel/bpf/trampoline.c         | 12 ++++++------
> > > >  8 files changed, 32 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > > > index 7d4af64e3982..b52549d18730 100644
> > > > --- a/arch/arm64/net/bpf_jit_comp.c
> > > > +++ b/arch/arm64/net/bpf_jit_comp.c
> > > > @@ -2167,7 +2167,8 @@ static int gen_branch_or_nop(enum aarch64_insn_branch_type type, void *ip,
> > > >   * locations during the patching process, making the patching process easier.
> > > >   */
> > > >  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
> > > > -                void *old_addr, void *new_addr)
> > > > +                void *old_addr, void *new_addr,
> > >
> > > [..]
> > >
> > > > +                bool checkip __maybe_unused)
> > >
> > > Any idea why only riscv and x86 do this check?
> >
> > so arm does the check as well, but needs the data from the lookup
> > to patch things properly.. but IIUC it does not suffer the same
> > issue because it does not implement direct tail calls [1] which
> > is used only on x86
> >
> > >
> > > Asking because maybe it makes sense to move this check into some
> > > new generic bpf_text_poke and call it in the places where you currently
> > > call checkip=true (and keep using bpf_arch_text_poke for checkip=false
> > > case).
> > >
> > > (don't see any issues with the current approach btw, just interested..)
> >
> > I tried to add new function for that, but it did not look good for arm
> > because it needs to do the lookup anyway
> >
> > hm maybe we could use new arch function that would cover the single
> > tail call 'text poke' update in prog_array_map_poke_run and would be
> > implemented only on x86 ... using __bpf_arch_text_poke directly
>
> looks like below change would be enough, I'll test and send new version

sg. I'm still not 100% sure why it's x86 only, I was (probably
wrongly?) assuming that at least arm64 jit is mostly on par with x86
:-)

Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Jiri Olsa]

On Wed, Nov 29, 2023 at 10:10:22AM -0800, Stanislav Fomichev wrote:
> On Wed, Nov 29, 2023 at 6:55 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Nov 29, 2023 at 03:05:21PM +0100, Jiri Olsa wrote:
> > > On Tue, Nov 28, 2023 at 01:24:14PM -0800, Stanislav Fomichev wrote:
> > > > On 11/28, Jiri Olsa wrote:
> > > > > We need to be able to skip ip address check for caller in following
> > > > > changes. Adding checkip argument to allow that.
> > > > >
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> > > > >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> > > > >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> > > > >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> > > > >  include/linux/bpf.h             |  2 +-
> > > > >  kernel/bpf/arraymap.c           |  8 ++++----
> > > > >  kernel/bpf/core.c               |  2 +-
> > > > >  kernel/bpf/trampoline.c         | 12 ++++++------
> > > > >  8 files changed, 32 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > > > > index 7d4af64e3982..b52549d18730 100644
> > > > > --- a/arch/arm64/net/bpf_jit_comp.c
> > > > > +++ b/arch/arm64/net/bpf_jit_comp.c
> > > > > @@ -2167,7 +2167,8 @@ static int gen_branch_or_nop(enum aarch64_insn_branch_type type, void *ip,
> > > > >   * locations during the patching process, making the patching process easier.
> > > > >   */
> > > > >  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
> > > > > -                void *old_addr, void *new_addr)
> > > > > +                void *old_addr, void *new_addr,
> > > >
> > > > [..]
> > > >
> > > > > +                bool checkip __maybe_unused)
> > > >
> > > > Any idea why only riscv and x86 do this check?
> > >
> > > so arm does the check as well, but needs the data from the lookup
> > > to patch things properly.. but IIUC it does not suffer the same
> > > issue because it does not implement direct tail calls [1] which
> > > is used only on x86
> > >
> > > >
> > > > Asking because maybe it makes sense to move this check into some
> > > > new generic bpf_text_poke and call it in the places where you currently
> > > > call checkip=true (and keep using bpf_arch_text_poke for checkip=false
> > > > case).
> > > >
> > > > (don't see any issues with the current approach btw, just interested..)
> > >
> > > I tried to add new function for that, but it did not look good for arm
> > > because it needs to do the lookup anyway
> > >
> > > hm maybe we could use new arch function that would cover the single
> > > tail call 'text poke' update in prog_array_map_poke_run and would be
> > > implemented only on x86 ... using __bpf_arch_text_poke directly
> >
> > looks like below change would be enough, I'll test and send new version
> 
> sg. I'm still not 100% sure why it's x86 only, I was (probably
> wrongly?) assuming that at least arm64 jit is mostly on par with x86
> :-)

AFAICS the direct tail calls are on x86, CI also seems to be ok with
that change.. I'll send it as formal patch

jirka

Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Ilya Leoshkevich]

On Tue, 2023-11-28 at 10:28 +0100, Jiri Olsa wrote:
> We need to be able to skip ip address check for caller in following
> changes. Adding checkip argument to allow that.
> 
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
>  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
>  arch/s390/net/bpf_jit_comp.c    |  3 ++-
>  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
>  include/linux/bpf.h             |  2 +-
>  kernel/bpf/arraymap.c           |  8 ++++----
>  kernel/bpf/core.c               |  2 +-
>  kernel/bpf/trampoline.c         | 12 ++++++------
>  8 files changed, 32 insertions(+), 27 deletions(-)

[...]

> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -435,19 +435,21 @@ static int __bpf_arch_text_poke(void *ip, enum
> bpf_text_poke_type t,
>  }
>  
>  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
> -                      void *old_addr, void *new_addr)
> +                      void *old_addr, void *new_addr, bool checkip)
>  {
> -       if (!is_kernel_text((long)ip) &&
> -           !is_bpf_text_address((long)ip))
> -               /* BPF poking in modules is not supported */
> -               return -EINVAL;
> +       if (checkip) {
> +               if (!is_kernel_text((long)ip) &&
> +                   !is_bpf_text_address((long)ip))
> +                       /* BPF poking in modules is not supported */
> +                       return -EINVAL;
>  
> -       /*
> -        * See emit_prologue(), for IBT builds the trampoline hook is
> preceded
> -        * with an ENDBR instruction.
> -        */
> -       if (is_endbr(*(u32 *)ip))
> -               ip += ENDBR_INSN_SIZE;
> +               /*
> +                * See emit_prologue(), for IBT builds the trampoline
> hook is preceded
> +                * with an ENDBR instruction.
> +                */
> +               if (is_endbr(*(u32 *)ip))
> +                       ip += ENDBR_INSN_SIZE;

Do we really want to skip the IP adjustment too?

> +       }
>  
>         return __bpf_arch_text_poke(ip, t, old_addr, new_addr);
>  }

[...]

Re: [PATCHv2 bpf 1/2] bpf: Add checkip argument to bpf_arch_text_poke

[Jiri Olsa]

On Fri, Dec 01, 2023 at 03:36:26PM +0100, Ilya Leoshkevich wrote:
> On Tue, 2023-11-28 at 10:28 +0100, Jiri Olsa wrote:
> > We need to be able to skip ip address check for caller in following
> > changes. Adding checkip argument to allow that.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> >  include/linux/bpf.h             |  2 +-
> >  kernel/bpf/arraymap.c           |  8 ++++----
> >  kernel/bpf/core.c               |  2 +-
> >  kernel/bpf/trampoline.c         | 12 ++++++------
> >  8 files changed, 32 insertions(+), 27 deletions(-)
> 
> [...]
> 
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -435,19 +435,21 @@ static int __bpf_arch_text_poke(void *ip, enum
> > bpf_text_poke_type t,
> >  }
> >  
> >  int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type t,
> > -                      void *old_addr, void *new_addr)
> > +                      void *old_addr, void *new_addr, bool checkip)
> >  {
> > -       if (!is_kernel_text((long)ip) &&
> > -           !is_bpf_text_address((long)ip))
> > -               /* BPF poking in modules is not supported */
> > -               return -EINVAL;
> > +       if (checkip) {
> > +               if (!is_kernel_text((long)ip) &&
> > +                   !is_bpf_text_address((long)ip))
> > +                       /* BPF poking in modules is not supported */
> > +                       return -EINVAL;
> >  
> > -       /*
> > -        * See emit_prologue(), for IBT builds the trampoline hook is
> > preceded
> > -        * with an ENDBR instruction.
> > -        */
> > -       if (is_endbr(*(u32 *)ip))
> > -               ip += ENDBR_INSN_SIZE;
> > +               /*
> > +                * See emit_prologue(), for IBT builds the trampoline
> > hook is preceded
> > +                * with an ENDBR instruction.
> > +                */
> > +               if (is_endbr(*(u32 *)ip))
> > +                       ip += ENDBR_INSN_SIZE;
> 
> Do we really want to skip the IP adjustment too?

the idea was that with __bpf_arch_text_poke you are aware of what you
are updating, so there's no need for extra checking

anyway this version got deprecated and I just sent v3 which is bit
different without this change

thanks,
jirka

> 
> > +       }
> >  
> >         return __bpf_arch_text_poke(ip, t, old_addr, new_addr);
> >  }
> 
> [...]

[PATCHv2 bpf 2/2] bpf: Fix prog_array_map_poke_run map poke update

[Jiri Olsa]

Lee pointed out issue found by syscaller [0] hitting BUG in prog array
map poke update in prog_array_map_poke_run function due to error value
returned from bpf_arch_text_poke function.

There's race window where bpf_arch_text_poke can fail due to missing
bpf program kallsym symbols, which is accounted for with check for
-EINVAL in that BUG_ON call.

The problem is that in such case we won't update the tail call jump
and cause imballance for the next tail call update check which will
fail with -EBUSY in bpf_arch_text_poke.

I'm hitting following race during the program load:

  CPU 0                             CPU 1

  bpf_prog_load
    bpf_check
      do_misc_fixups
        prog_array_map_poke_track

                                    map_update_elem
                                      bpf_fd_array_map_update_elem
                                        prog_array_map_poke_run

                                          bpf_arch_text_poke returns -EINVAL

    bpf_prog_kallsyms_add

After bpf_arch_text_poke (CPU 1) fails to update the tail call jump, the next
poke update fails on expected jump instruction check in bpf_arch_text_poke
with -EBUSY and triggers the BUG_ON in prog_array_map_poke_run.

Similar race exists on the program unload.

Fixing this by calling bpf_arch_text_poke with 'checkip=false' to skip the
bpf symbol check like we do in bpf_tail_call_direct_fixup. This way the
prog_array_map_poke_run does not depend on bpf program having the kallsym
symbol in place.

[0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810

Cc: Lee Jones <lee@kernel.org>
Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
Reported-by: syzbot+97a4fe20470e9bc30810@syzkaller.appspotmail.com
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/bpf/arraymap.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 7ba389f7212f..b194282eacbb 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -1044,20 +1044,11 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 			 *    activated, so tail call updates can arrive from here
 			 *    while JIT is still finishing its final fixup for
 			 *    non-activated poke entries.
-			 * 3) On program teardown, the program's kallsym entry gets
-			 *    removed out of RCU callback, but we can only untrack
-			 *    from sleepable context, therefore bpf_arch_text_poke()
-			 *    might not see that this is in BPF text section and
-			 *    bails out with -EINVAL. As these are unreachable since
-			 *    RCU grace period already passed, we simply skip them.
-			 * 4) Also programs reaching refcount of zero while patching
+			 * 3) Also programs reaching refcount of zero while patching
 			 *    is in progress is okay since we're protected under
 			 *    poke_mutex and untrack the programs before the JIT
-			 *    buffer is freed. When we're still in the middle of
-			 *    patching and suddenly kallsyms entry of the program
-			 *    gets evicted, we just skip the rest which is fine due
-			 *    to point 3).
-			 * 5) Any other error happening below from bpf_arch_text_poke()
+			 *    buffer is freed.
+			 * 4) Any error happening below from bpf_arch_text_poke()
 			 *    is a unexpected bug.
 			 */
 			if (!READ_ONCE(poke->tailcall_target_stable))
@@ -1075,21 +1066,21 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 			if (new) {
 				ret = bpf_arch_text_poke(poke->tailcall_target,
 							 BPF_MOD_JUMP,
-							 old_addr, new_addr, true);
-				BUG_ON(ret < 0 && ret != -EINVAL);
+							 old_addr, new_addr, false);
+				BUG_ON(ret < 0);
 				if (!old) {
 					ret = bpf_arch_text_poke(poke->tailcall_bypass,
 								 BPF_MOD_JUMP,
 								 poke->bypass_addr,
-								 NULL, true);
-					BUG_ON(ret < 0 && ret != -EINVAL);
+								 NULL, false);
+					BUG_ON(ret < 0);
 				}
 			} else {
 				ret = bpf_arch_text_poke(poke->tailcall_bypass,
 							 BPF_MOD_JUMP,
 							 old_bypass_addr,
-							 poke->bypass_addr, true);
-				BUG_ON(ret < 0 && ret != -EINVAL);
+							 poke->bypass_addr, false);
+				BUG_ON(ret < 0);
 				/* let other CPUs finish the execution of program
 				 * so that it will not possible to expose them
 				 * to invalid nop, stack unwind, nop state
@@ -1098,8 +1089,8 @@ static void prog_array_map_poke_run(struct bpf_map *map, u32 key,
 					synchronize_rcu();
 				ret = bpf_arch_text_poke(poke->tailcall_target,
 							 BPF_MOD_JUMP,
-							 old_addr, NULL, true);
-				BUG_ON(ret < 0 && ret != -EINVAL);
+							 old_addr, NULL, false);
+				BUG_ON(ret < 0);
 			}
 		}
 	}
-- 
2.43.0

Re: [PATCHv2 bpf 0/2] bpf: Fix prog_array_map_poke_run map poke update

[Ilya Leoshkevich]

On Tue, 2023-11-28 at 10:28 +0100, Jiri Olsa wrote:
> hi,
> this patchset fixes the issue reported in [0].
> 
> For the actual fix in patch 2 I'm changing bpf_arch_text_poke to
> allow to skip
> ip address check in patch 1. I considered adding separate function
> for that,
> but because each arch implementation is bit different, adding extra
> arg seemed
> like better option.
> 
> v2 changes:
>   - make it work for other archs
> 
> thanks,
> jirka
> 
> 
> [0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
> ---
> Jiri Olsa (2):
>       bpf: Add checkip argument to bpf_arch_text_poke
>       bpf, x64: Fix prog_array_map_poke_run map poke update
> 
>  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
>  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
>  arch/s390/net/bpf_jit_comp.c    |  3 ++-
>  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
>  include/linux/bpf.h             |  2 +-
>  kernel/bpf/arraymap.c           | 31 +++++++++++--------------------
>  kernel/bpf/core.c               |  2 +-
>  kernel/bpf/trampoline.c         | 12 ++++++------
>  8 files changed, 39 insertions(+), 43 deletions(-)

Would it be possible to add a minimized version of the reproducer as a
testcase?

Re: [PATCHv2 bpf 0/2] bpf: Fix prog_array_map_poke_run map poke update

[Jiri Olsa]

On Tue, Nov 28, 2023 at 11:44:33PM +0100, Ilya Leoshkevich wrote:
> On Tue, 2023-11-28 at 10:28 +0100, Jiri Olsa wrote:
> > hi,
> > this patchset fixes the issue reported in [0].
> > 
> > For the actual fix in patch 2 I'm changing bpf_arch_text_poke to
> > allow to skip
> > ip address check in patch 1. I considered adding separate function
> > for that,
> > but because each arch implementation is bit different, adding extra
> > arg seemed
> > like better option.
> > 
> > v2 changes:
> >   - make it work for other archs
> > 
> > thanks,
> > jirka
> > 
> > 
> > [0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
> > ---
> > Jiri Olsa (2):
> >       bpf: Add checkip argument to bpf_arch_text_poke
> >       bpf, x64: Fix prog_array_map_poke_run map poke update
> > 
> >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> >  include/linux/bpf.h             |  2 +-
> >  kernel/bpf/arraymap.c           | 31 +++++++++++--------------------
> >  kernel/bpf/core.c               |  2 +-
> >  kernel/bpf/trampoline.c         | 12 ++++++------
> >  8 files changed, 39 insertions(+), 43 deletions(-)
> 
> Would it be possible to add a minimized version of the reproducer as a
> testcase?

there's reproducer I used in here:
  https://syzkaller.appspot.com/text?tag=ReproC&x=1397180f680000

I can try, but not sure I'll be able to come up with something that
would fit as testcase.. I'll check

jirka

Re: [PATCHv2 bpf 0/2] bpf: Fix prog_array_map_poke_run map poke update

[Jiri Olsa]

On Wed, Nov 29, 2023 at 02:23:04PM +0100, Jiri Olsa wrote:
> On Tue, Nov 28, 2023 at 11:44:33PM +0100, Ilya Leoshkevich wrote:
> > On Tue, 2023-11-28 at 10:28 +0100, Jiri Olsa wrote:
> > > hi,
> > > this patchset fixes the issue reported in [0].
> > > 
> > > For the actual fix in patch 2 I'm changing bpf_arch_text_poke to
> > > allow to skip
> > > ip address check in patch 1. I considered adding separate function
> > > for that,
> > > but because each arch implementation is bit different, adding extra
> > > arg seemed
> > > like better option.
> > > 
> > > v2 changes:
> > >   - make it work for other archs
> > > 
> > > thanks,
> > > jirka
> > > 
> > > 
> > > [0] https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
> > > ---
> > > Jiri Olsa (2):
> > >       bpf: Add checkip argument to bpf_arch_text_poke
> > >       bpf, x64: Fix prog_array_map_poke_run map poke update
> > > 
> > >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> > >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> > >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> > >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> > >  include/linux/bpf.h             |  2 +-
> > >  kernel/bpf/arraymap.c           | 31 +++++++++++--------------------
> > >  kernel/bpf/core.c               |  2 +-
> > >  kernel/bpf/trampoline.c         | 12 ++++++------
> > >  8 files changed, 39 insertions(+), 43 deletions(-)
> > 
> > Would it be possible to add a minimized version of the reproducer as a
> > testcase?
> 
> there's reproducer I used in here:
>   https://syzkaller.appspot.com/text?tag=ReproC&x=1397180f680000
> 
> I can try, but not sure I'll be able to come up with something that
> would fit as testcase.. I'll check

the test below reproduces it for me.. the only tricky part is that
I need to repeat the loop 10 times to trigger that on my setup..
which is not terrible, but not great for a test I think

jirka


---
diff --git a/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
new file mode 100644
index 000000000000..c18751677811
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/tailcall_poke.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <unistd.h>
+#include <test_progs.h>
+#include "tailcall_poke.skel.h"
+
+#define JMP_TABLE "/sys/fs/bpf/jmp_table"
+
+static int thread_exit;
+
+static void *update(void *arg)
+{
+	__u32 zero = 0, prog1_fd, prog2_fd, map_fd;
+	struct tailcall_poke *call = arg;
+
+	map_fd = bpf_map__fd(call->maps.jmp_table);
+	prog1_fd = bpf_program__fd(call->progs.call1);
+	prog2_fd = bpf_program__fd(call->progs.call2);
+
+	while (!thread_exit) {
+		bpf_map_update_elem(map_fd, &zero, &prog1_fd, BPF_ANY);
+		bpf_map_update_elem(map_fd, &zero, &prog2_fd, BPF_ANY);
+	}
+
+	return NULL;
+}
+
+void test_tailcall_poke(void)
+{
+	struct tailcall_poke *call, *test;
+	int err, cnt = 10;
+	pthread_t thread;
+
+	unlink(JMP_TABLE);
+
+	call = tailcall_poke__open_and_load();
+	if (!ASSERT_OK_PTR(call, "tailcall_poke__open"))
+		return;
+
+	err = bpf_map__pin(call->maps.jmp_table, JMP_TABLE);
+	if (!ASSERT_OK(err, "bpf_map__pin"))
+		goto out;
+
+	err = pthread_create(&thread, NULL, update, call);
+	if (!ASSERT_OK(err, "new toggler"))
+		goto out;
+
+	while (cnt--) {
+		test = tailcall_poke__open();
+		if (!ASSERT_OK_PTR(test, "tailcall_poke__open"))
+			break;
+
+		err = bpf_map__set_pin_path(test->maps.jmp_table, JMP_TABLE);
+		if (!ASSERT_OK(err, "bpf_map__pin")) {
+			tailcall_poke__destroy(test);
+			break;
+		}
+
+		bpf_program__set_autoload(test->progs.test, true);
+		bpf_program__set_autoload(test->progs.call1, false);
+		bpf_program__set_autoload(test->progs.call2, false);
+
+		err = tailcall_poke__load(test);
+		if (!ASSERT_OK(err, "tailcall_poke__load")) {
+			tailcall_poke__destroy(test);
+			break;
+		}
+
+		tailcall_poke__destroy(test);
+	}
+
+	thread_exit = 1;
+	ASSERT_OK(pthread_join(thread, NULL), "pthread_join");
+
+out:
+	bpf_map__unpin(call->maps.jmp_table, JMP_TABLE);
+	tailcall_poke__destroy(call);
+}
diff --git a/tools/testing/selftests/bpf/progs/tailcall_poke.c b/tools/testing/selftests/bpf/progs/tailcall_poke.c
new file mode 100644
index 000000000000..d4cf63c7db01
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_poke.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+        __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+        __uint(max_entries, 1);
+        __uint(key_size, sizeof(__u32));
+        __uint(value_size, sizeof(__u32));
+} jmp_table SEC(".maps");
+
+SEC("?fentry/bpf_fentry_test1")
+int BPF_PROG(test, int a)
+{
+	bpf_tail_call_static(ctx, &jmp_table, 0);
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(call1, int a)
+{
+	return 0;
+}
+
+SEC("fentry/bpf_fentry_test1")
+int BPF_PROG(call2, int a)
+{
+	return 0;
+}

Re: [PATCHv2 bpf 0/2] bpf: Fix prog_array_map_poke_run map poke update

[Ilya Leoshkevich]

On Fri, 2023-12-01 at 14:13 +0100, Jiri Olsa wrote:
> On Wed, Nov 29, 2023 at 02:23:04PM +0100, Jiri Olsa wrote:
> > On Tue, Nov 28, 2023 at 11:44:33PM +0100, Ilya Leoshkevich wrote:
> > > On Tue, 2023-11-28 at 10:28 +0100, Jiri Olsa wrote:
> > > > hi,
> > > > this patchset fixes the issue reported in [0].
> > > > 
> > > > For the actual fix in patch 2 I'm changing bpf_arch_text_poke
> > > > to
> > > > allow to skip
> > > > ip address check in patch 1. I considered adding separate
> > > > function
> > > > for that,
> > > > but because each arch implementation is bit different, adding
> > > > extra
> > > > arg seemed
> > > > like better option.
> > > > 
> > > > v2 changes:
> > > >   - make it work for other archs
> > > > 
> > > > thanks,
> > > > jirka
> > > > 
> > > > 
> > > > [0]
> > > > https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
> > > > ---
> > > > Jiri Olsa (2):
> > > >       bpf: Add checkip argument to bpf_arch_text_poke
> > > >       bpf, x64: Fix prog_array_map_poke_run map poke update
> > > > 
> > > >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> > > >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> > > >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> > > >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> > > >  include/linux/bpf.h             |  2 +-
> > > >  kernel/bpf/arraymap.c           | 31 +++++++++++--------------
> > > > ------
> > > >  kernel/bpf/core.c               |  2 +-
> > > >  kernel/bpf/trampoline.c         | 12 ++++++------
> > > >  8 files changed, 39 insertions(+), 43 deletions(-)
> > > 
> > > Would it be possible to add a minimized version of the reproducer
> > > as a
> > > testcase?
> > 
> > there's reproducer I used in here:
> >   https://syzkaller.appspot.com/text?tag=ReproC&x=1397180f680000
> > 
> > I can try, but not sure I'll be able to come up with something that
> > would fit as testcase.. I'll check
> 
> the test below reproduces it for me.. the only tricky part is that
> I need to repeat the loop 10 times to trigger that on my setup..
> which is not terrible, but not great for a test I think
> 
> jirka

The test looks useful to me. I think having magic repetition counts
like this 10 here is almost inevitable when trying to reproduce race
conditions. The test also runs quickly for me. You can have my

Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>

in case you decide to make a formal patch.

Re: [PATCHv2 bpf 0/2] bpf: Fix prog_array_map_poke_run map poke update

[Jiri Olsa]

On Fri, Dec 01, 2023 at 03:31:57PM +0100, Ilya Leoshkevich wrote:
> On Fri, 2023-12-01 at 14:13 +0100, Jiri Olsa wrote:
> > On Wed, Nov 29, 2023 at 02:23:04PM +0100, Jiri Olsa wrote:
> > > On Tue, Nov 28, 2023 at 11:44:33PM +0100, Ilya Leoshkevich wrote:
> > > > On Tue, 2023-11-28 at 10:28 +0100, Jiri Olsa wrote:
> > > > > hi,
> > > > > this patchset fixes the issue reported in [0].
> > > > > 
> > > > > For the actual fix in patch 2 I'm changing bpf_arch_text_poke
> > > > > to
> > > > > allow to skip
> > > > > ip address check in patch 1. I considered adding separate
> > > > > function
> > > > > for that,
> > > > > but because each arch implementation is bit different, adding
> > > > > extra
> > > > > arg seemed
> > > > > like better option.
> > > > > 
> > > > > v2 changes:
> > > > >   - make it work for other archs
> > > > > 
> > > > > thanks,
> > > > > jirka
> > > > > 
> > > > > 
> > > > > [0]
> > > > > https://syzkaller.appspot.com/bug?extid=97a4fe20470e9bc30810
> > > > > ---
> > > > > Jiri Olsa (2):
> > > > >       bpf: Add checkip argument to bpf_arch_text_poke
> > > > >       bpf, x64: Fix prog_array_map_poke_run map poke update
> > > > > 
> > > > >  arch/arm64/net/bpf_jit_comp.c   |  3 ++-
> > > > >  arch/riscv/net/bpf_jit_comp64.c |  5 +++--
> > > > >  arch/s390/net/bpf_jit_comp.c    |  3 ++-
> > > > >  arch/x86/net/bpf_jit_comp.c     | 24 +++++++++++++-----------
> > > > >  include/linux/bpf.h             |  2 +-
> > > > >  kernel/bpf/arraymap.c           | 31 +++++++++++--------------
> > > > > ------
> > > > >  kernel/bpf/core.c               |  2 +-
> > > > >  kernel/bpf/trampoline.c         | 12 ++++++------
> > > > >  8 files changed, 39 insertions(+), 43 deletions(-)
> > > > 
> > > > Would it be possible to add a minimized version of the reproducer
> > > > as a
> > > > testcase?
> > > 
> > > there's reproducer I used in here:
> > >   https://syzkaller.appspot.com/text?tag=ReproC&x=1397180f680000
> > > 
> > > I can try, but not sure I'll be able to come up with something that
> > > would fit as testcase.. I'll check
> > 
> > the test below reproduces it for me.. the only tricky part is that
> > I need to repeat the loop 10 times to trigger that on my setup..
> > which is not terrible, but not great for a test I think
> > 
> > jirka
> 
> The test looks useful to me. I think having magic repetition counts
> like this 10 here is almost inevitable when trying to reproduce race
> conditions. The test also runs quickly for me. You can have my
> 
> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
> 
> in case you decide to make a formal patch.

great, thanks

jirka