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

[Jiri Olsa <olsajiri@gmail.com>]

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