[PATCH bpf-next 0/2] s390/bpf: Remove the orig_call NULL check

[PATCH bpf-next 0/2] s390/bpf: Remove the orig_call NULL check

[Ilya Leoshkevich]

Hi,

I've been looking at fixing the tailcall_bpf2bpf_hierarchy failures on
s390. One of the challenges is that when a BPF trampoline calls a BPF
prog A, the prologue of A sets the tail call count to 0. Therefore it
would be useful to know whether the trampoline is attached to some
other BPF prog B, in which case A should be called using an offset
equal to tail_call_start, bypassing the tail call count initialization.

The trampoline attachment point is passed to trampoline functions via
the orig_call variable. Unfortunately in the case of calculating the
size of a struct_ops trampoline it's NULL, and I could not think of a
good reason to have it this way. This series makes it always non-NULL.

Best regards,
Ilya

Ilya Leoshkevich (2):
  bpf: Pass the same orig_call value to trampoline functions
  s390/bpf: Remove the orig_call NULL check

 arch/s390/net/bpf_jit_comp.c | 5 ++---
 kernel/bpf/bpf_struct_ops.c  | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

-- 
2.49.0

[PATCH bpf-next 1/2] bpf: Pass the same orig_call value to trampoline functions

[Ilya Leoshkevich]

There is currently some confusion in the s390x JIT regarding whether
orig_call can be NULL and what that means. Originally the NULL value
was used to distinguish the struct_ops case, but this was superseded by
BPF_TRAMP_F_INDIRECT (see commit 0c970ed2f87c ("s390/bpf: Fix indirect
trampoline generation").

The remaining reason to have this check is that NULL can actually be
passed to the arch_bpf_trampoline_size() call - but not to the
respective arch_prepare_bpf_trampoline()! call - by
bpf_struct_ops_prepare_trampoline().

Remove this asymmetry by passing stub_func to both functions, so that
JITs may rely on orig_call never being NULL.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 kernel/bpf/bpf_struct_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index db13ee70d94d..96113633e391 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -601,7 +601,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
 	if (model->ret_size > 0)
 		flags |= BPF_TRAMP_F_RET_FENTRY_RET;
 
-	size = arch_bpf_trampoline_size(model, flags, tlinks, NULL);
+	size = arch_bpf_trampoline_size(model, flags, tlinks, stub_func);
 	if (size <= 0)
 		return size ? : -EFAULT;
 
-- 
2.49.0

Re: [PATCH bpf-next 1/2] bpf: Pass the same orig_call value to trampoline functions

[Martin KaFai Lau]

On 5/12/25 1:57 PM, Ilya Leoshkevich wrote:
> There is currently some confusion in the s390x JIT regarding whether
> orig_call can be NULL and what that means. Originally the NULL value
> was used to distinguish the struct_ops case, but this was superseded by
> BPF_TRAMP_F_INDIRECT (see commit 0c970ed2f87c ("s390/bpf: Fix indirect
> trampoline generation").
> 
> The remaining reason to have this check is that NULL can actually be
> passed to the arch_bpf_trampoline_size() call - but not to the
> respective arch_prepare_bpf_trampoline()! call - by
> bpf_struct_ops_prepare_trampoline().
> 
> Remove this asymmetry by passing stub_func to both functions, so that
> JITs may rely on orig_call never being NULL.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   kernel/bpf/bpf_struct_ops.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index db13ee70d94d..96113633e391 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c
> @@ -601,7 +601,7 @@ int bpf_struct_ops_prepare_trampoline(struct bpf_tramp_links *tlinks,
>   	if (model->ret_size > 0)
>   		flags |= BPF_TRAMP_F_RET_FENTRY_RET;
>   
> -	size = arch_bpf_trampoline_size(model, flags, tlinks, NULL);
> +	size = arch_bpf_trampoline_size(model, flags, tlinks, stub_func);

The change looks ok but not sure why it is needed.

I can see why stub_func is needed to generate the final image in 
arch_prepare_bpf_trampoline() in x86. The "arch_bpf_trampoline_size()" here is 
generating a temporary image, so NULL or not doesn't seem to matter.

Does the s390 jit need to use the actual stub_func address somewhere in the 
temporary and/or final image?

Re: [PATCH bpf-next 1/2] bpf: Pass the same orig_call value to trampoline functions

[Ilya Leoshkevich]

On Wed, 2025-05-14 at 14:26 -0700, Martin KaFai Lau wrote:
> On 5/12/25 1:57 PM, Ilya Leoshkevich wrote:
> > There is currently some confusion in the s390x JIT regarding
> > whether
> > orig_call can be NULL and what that means. Originally the NULL
> > value
> > was used to distinguish the struct_ops case, but this was
> > superseded by
> > BPF_TRAMP_F_INDIRECT (see commit 0c970ed2f87c ("s390/bpf: Fix
> > indirect
> > trampoline generation").
> > 
> > The remaining reason to have this check is that NULL can actually
> > be
> > passed to the arch_bpf_trampoline_size() call - but not to the
> > respective arch_prepare_bpf_trampoline()! call - by
> > bpf_struct_ops_prepare_trampoline().
> > 
> > Remove this asymmetry by passing stub_func to both functions, so
> > that
> > JITs may rely on orig_call never being NULL.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >   kernel/bpf/bpf_struct_ops.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/bpf/bpf_struct_ops.c
> > b/kernel/bpf/bpf_struct_ops.c
> > index db13ee70d94d..96113633e391 100644
> > --- a/kernel/bpf/bpf_struct_ops.c
> > +++ b/kernel/bpf/bpf_struct_ops.c
> > @@ -601,7 +601,7 @@ int bpf_struct_ops_prepare_trampoline(struct
> > bpf_tramp_links *tlinks,
> >   	if (model->ret_size > 0)
> >   		flags |= BPF_TRAMP_F_RET_FENTRY_RET;
> >   
> > -	size = arch_bpf_trampoline_size(model, flags, tlinks,
> > NULL);
> > +	size = arch_bpf_trampoline_size(model, flags, tlinks,
> > stub_func);
> 
> The change looks ok but not sure why it is needed.
> 
> I can see why stub_func is needed to generate the final image in 
> arch_prepare_bpf_trampoline() in x86. The
> "arch_bpf_trampoline_size()" here is 
> generating a temporary image, so NULL or not doesn't seem to matter.
> 
> Does the s390 jit need to use the actual stub_func address somewhere
> in the 
> temporary and/or final image?

Not right now, however, in the future I would like to check whether
orig_call points to a BPF prog. I have explained the rationale behind
this in the series description.

Purely practical issues aside, currently it's unclear what
orig_func == NULL means. It had meaning in the past, but not anymore.
I think it would be good to remove this uncertainty and state that
today orig_func is never NULL.

Furthermore, a hypothetical JIT may produce different instruction
sequences for loading certain constants. Suppose we wanted to load
the value of orig_call into a register. Then in the NULL case a JIT
could generate:

  xgr %r1,%r1

and in the non-NULL case:

  llihf %r1,<high>
  oilf %r1,<low>

As you mentioned, arch_bpf_trampoline_size() generates a temporary
image, and in this case it would have a different size. So this
asymmetry can be a potential footgun.

Re: [PATCH bpf-next 1/2] bpf: Pass the same orig_call value to trampoline functions

[Martin KaFai Lau]

On 5/14/25 3:46 PM, Ilya Leoshkevich wrote:
> On Wed, 2025-05-14 at 14:26 -0700, Martin KaFai Lau wrote:
>> On 5/12/25 1:57 PM, Ilya Leoshkevich wrote:
>>> There is currently some confusion in the s390x JIT regarding
>>> whether
>>> orig_call can be NULL and what that means. Originally the NULL
>>> value
>>> was used to distinguish the struct_ops case, but this was
>>> superseded by
>>> BPF_TRAMP_F_INDIRECT (see commit 0c970ed2f87c ("s390/bpf: Fix
>>> indirect
>>> trampoline generation").
>>>
>>> The remaining reason to have this check is that NULL can actually
>>> be
>>> passed to the arch_bpf_trampoline_size() call - but not to the
>>> respective arch_prepare_bpf_trampoline()! call - by
>>> bpf_struct_ops_prepare_trampoline().
>>>
>>> Remove this asymmetry by passing stub_func to both functions, so
>>> that
>>> JITs may rely on orig_call never being NULL.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    kernel/bpf/bpf_struct_ops.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/bpf_struct_ops.c
>>> b/kernel/bpf/bpf_struct_ops.c
>>> index db13ee70d94d..96113633e391 100644
>>> --- a/kernel/bpf/bpf_struct_ops.c
>>> +++ b/kernel/bpf/bpf_struct_ops.c
>>> @@ -601,7 +601,7 @@ int bpf_struct_ops_prepare_trampoline(struct
>>> bpf_tramp_links *tlinks,
>>>    	if (model->ret_size > 0)
>>>    		flags |= BPF_TRAMP_F_RET_FENTRY_RET;
>>>    
>>> -	size = arch_bpf_trampoline_size(model, flags, tlinks,
>>> NULL);
>>> +	size = arch_bpf_trampoline_size(model, flags, tlinks,
>>> stub_func);
>>
>> The change looks ok but not sure why it is needed.
>>
>> I can see why stub_func is needed to generate the final image in
>> arch_prepare_bpf_trampoline() in x86. The
>> "arch_bpf_trampoline_size()" here is
>> generating a temporary image, so NULL or not doesn't seem to matter.
>>
>> Does the s390 jit need to use the actual stub_func address somewhere
>> in the
>> temporary and/or final image?
> 
> Not right now, however, in the future I would like to check whether
> orig_call points to a BPF prog. I have explained the rationale behind
> this in the series description.
> 
> Purely practical issues aside, currently it's unclear what
> orig_func == NULL means. It had meaning in the past, but not anymore.
> I think it would be good to remove this uncertainty and state that
> today orig_func is never NULL.

For the bpf_struct_ops trampoline image, there is no need to invoke the 
orig_call, so it had been NULL for both temporary and final image cases until 
the recent cfi fix in x86.

I was asking because it feels like the jit might be doing something incorrect 
(or at least unnecessary) for the bpf_struct_ops if it needs to use the 
orig_call address. I don't know much of the s390 jit though.

The change looks fine from the bpf_struct_ops pov. Always pass "stub_func" is 
more consistent also, so

Acked-by: Martin KaFai Lau <martin.lau@kernel.org>

I don't know enough about the tailcall_bpf2bpf_hierarchy as stated in the commit 
message, so I will defer to others to comment.

> Furthermore, a hypothetical JIT may produce different instruction
> sequences for loading certain constants. Suppose we wanted to load
> the value of orig_call into a register. Then in the NULL case a JIT
> could generate:
> 
>    xgr %r1,%r1
> 
> and in the non-NULL case:
> 
>    llihf %r1,<high>
>    oilf %r1,<low>
> 
> As you mentioned, arch_bpf_trampoline_size() generates a temporary
> image, and in this case it would have a different size. So this
> asymmetry can be a potential footgun.

[PATCH bpf-next 2/2] s390/bpf: Remove the orig_call NULL check

[Ilya Leoshkevich]

Now that orig_call can never be NULL, remove the respective check.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 arch/s390/net/bpf_jit_comp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c
index 0776dfde2dba..4618f706067d 100644
--- a/arch/s390/net/bpf_jit_comp.c
+++ b/arch/s390/net/bpf_jit_comp.c
@@ -2585,9 +2585,8 @@ static int __arch_prepare_bpf_trampoline(struct bpf_tramp_image *im,
 	if (nr_stack_args > MAX_NR_STACK_ARGS)
 		return -ENOTSUPP;
 
-	/* Return to %r14, since func_addr and %r0 are not available. */
-	if ((!func_addr && !(flags & BPF_TRAMP_F_ORIG_STACK)) ||
-	    (flags & BPF_TRAMP_F_INDIRECT))
+	/* Return to %r14 in the struct_ops case. */
+	if (flags & BPF_TRAMP_F_INDIRECT)
 		flags |= BPF_TRAMP_F_SKIP_FRAME;
 
 	/*
-- 
2.49.0

Re: [PATCH bpf-next 0/2] s390/bpf: Remove the orig_call NULL check

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 12 May 2025 22:57:29 +0200 you wrote:
> Hi,
> 
> I've been looking at fixing the tailcall_bpf2bpf_hierarchy failures on
> s390. One of the challenges is that when a BPF trampoline calls a BPF
> prog A, the prologue of A sets the tail call count to 0. Therefore it
> would be useful to know whether the trampoline is attached to some
> other BPF prog B, in which case A should be called using an offset
> equal to tail_call_start, bypassing the tail call count initialization.
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] bpf: Pass the same orig_call value to trampoline functions
    https://git.kernel.org/bpf/bpf-next/c/94bde253d3ae
  - [bpf-next,2/2] s390/bpf: Remove the orig_call NULL check
    https://git.kernel.org/bpf/bpf-next/c/8e57cf09c84c

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