[PATCH RFC] bpf: update current instruction on patching

[PATCH RFC] bpf: update current instruction on patching

[Yauheni Kaliuta]

On code patching it may require to update branch destinations if the
code size changed. bpf_adj_delta_to_imm/off increments offset only
if the patched area is after the branch instruction. But it's
possible, that the patched area itself is a branch instruction and
requires destination update.

The problem was triggered by bpf selftest

test_progs -t global_funcs

on s390, where the very first "call" instruction is patched from
verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.

The patch includes current instruction to the condition check.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 kernel/bpf/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ed0b3578867c..b0a9a22491a5 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, s32 end_old,
 	s32 delta = end_new - end_old;
 	s64 imm = insn->imm;
 
-	if (curr < pos && curr + imm + 1 >= end_old)
+	if (curr <= pos && curr + imm + 1 >= end_old)
 		imm += delta;
 	else if (curr >= end_new && curr + imm + 1 < end_new)
 		imm -= delta;
@@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, s32 end_old,
 	s32 delta = end_new - end_old;
 	s32 off = insn->off;
 
-	if (curr < pos && curr + off + 1 >= end_old)
+	if (curr <= pos && curr + off + 1 >= end_old)
 		off += delta;
 	else if (curr >= end_new && curr + off + 1 < end_new)
 		off -= delta;
-- 
2.26.2

Re: [PATCH RFC] bpf: update current instruction on patching

[Yauheni Kaliuta]

Breaks
#76/u bounds check mixed 32bit and 64bit arithmetic. test2


On Thu, Sep 3, 2020 at 6:02 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> On code patching it may require to update branch destinations if the
> code size changed. bpf_adj_delta_to_imm/off increments offset only
> if the patched area is after the branch instruction. But it's
> possible, that the patched area itself is a branch instruction and
> requires destination update.
>
> The problem was triggered by bpf selftest
>
> test_progs -t global_funcs
>
> on s390, where the very first "call" instruction is patched from
> verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.
>
> The patch includes current instruction to the condition check.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  kernel/bpf/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index ed0b3578867c..b0a9a22491a5 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, s32 end_old,
>         s32 delta = end_new - end_old;
>         s64 imm = insn->imm;
>
> -       if (curr < pos && curr + imm + 1 >= end_old)
> +       if (curr <= pos && curr + imm + 1 >= end_old)
>                 imm += delta;
>         else if (curr >= end_new && curr + imm + 1 < end_new)
>                 imm -= delta;
> @@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, s32 end_old,
>         s32 delta = end_new - end_old;
>         s32 off = insn->off;
>
> -       if (curr < pos && curr + off + 1 >= end_old)
> +       if (curr <= pos && curr + off + 1 >= end_old)
>                 off += delta;
>         else if (curr >= end_new && curr + off + 1 < end_new)
>                 off -= delta;
> --
> 2.26.2
>


-- 
WBR, Yauheni

Re: [PATCH RFC] bpf: update current instruction on patching

[Daniel Borkmann]

On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
> On code patching it may require to update branch destinations if the
> code size changed. bpf_adj_delta_to_imm/off increments offset only
> if the patched area is after the branch instruction. But it's
> possible, that the patched area itself is a branch instruction and
> requires destination update.

Could you provide a concrete example and walk us through? I'm probably
missing something but if the patchlet contains a branch instruction, then
it should be 'self-contained'. In the sense that the patchlet is a 'black
box' that replaces 1 insns with n insns but there is no awareness what's
inside these insns and hence no fixup for that inside bpf_patch_insn_data().
So, if we take an existing branch insns from the code, move it into the
patchlet and extend beginning or end, then it feels more like a bug to the
one that called bpf_patch_insn_data(), aka zext code here. Bit puzzled why
this is only seen now, my impression was that Ilya was running s390x the
BPF selftests quite recently?

> The problem was triggered by bpf selftest
> 
> test_progs -t global_funcs
> 
> on s390, where the very first "call" instruction is patched from
> verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.
> 
> The patch includes current instruction to the condition check.
> 
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>   kernel/bpf/core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index ed0b3578867c..b0a9a22491a5 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, s32 end_old,
>   	s32 delta = end_new - end_old;
>   	s64 imm = insn->imm;
>   
> -	if (curr < pos && curr + imm + 1 >= end_old)
> +	if (curr <= pos && curr + imm + 1 >= end_old)
>   		imm += delta;
>   	else if (curr >= end_new && curr + imm + 1 < end_new)
>   		imm -= delta;
> @@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, s32 end_old,
>   	s32 delta = end_new - end_old;
>   	s32 off = insn->off;
>   
> -	if (curr < pos && curr + off + 1 >= end_old)
> +	if (curr <= pos && curr + off + 1 >= end_old)
>   		off += delta;
>   	else if (curr >= end_new && curr + off + 1 < end_new)
>   		off -= delta;
> 

Re: [PATCH RFC] bpf: update current instruction on patching

[Yauheni Kaliuta]

On Thu, Sep 3, 2020 at 6:10 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
> > On code patching it may require to update branch destinations if the
> > code size changed. bpf_adj_delta_to_imm/off increments offset only
> > if the patched area is after the branch instruction. But it's
> > possible, that the patched area itself is a branch instruction and
> > requires destination update.
>
> Could you provide a concrete example and walk us through? I'm probably
> missing something but if the patchlet contains a branch instruction, then
> it should be 'self-contained'. In the sense that the patchlet is a 'black
> box' that replaces 1 insns with n insns but there is no awareness what's
> inside these insns and hence no fixup for that inside bpf_patch_insn_data().

The code is
Disassembly of section classifier/test:

0000000000000000 test_cls:
       0:       85 01 00 00 ff ff ff ff call -1
                0000000000000000:  R_BPF_64_32  f7
       1:       95 00 00 00 00 00 00 00 exit
0000000000000000 f1:
       0:       61 01 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
       1:       95 00 00 00 00 00 00 00 exit
[...]
00000000000000a8 f7:
      21:       85 01 00 00 ff ff ff ff call -1
                00000000000000a8:  R_BPF_64_32  f6
      22:       95 00 00 00 00 00 00 00 exit

Before the patching the bytecode is:

00000000: 85 01 00 00 00 00 00 16 95 00 00 00 00 00 00 00
00000010: 61 01 00 00 00 00 00 00 95 00 00 00 00 00 00 00
[...]

It becomes


00000000: 85 01 00 00 00 00 00 2b bc 00 00 00 00 00 00 01
00000010: 95 00 00 00 00 00 00 00 61 01 00 80 00 00 00 00

at the end, the 2b offset is incorrect.

With that zext patching the code "85 01 00 00 00 00 00 16" is replaced
with "85 01 00 00 00 00 00 16 bc 00 00 00 00 00 00 01", 0x16 is not
changed, but the real offset has changed.

> So, if we take an existing branch insns from the code, move it into the
> patchlet and extend beginning or end, then it feels more like a bug to the
> one that called bpf_patch_insn_data(), aka zext code here. Bit puzzled why
> this is only seen now, my impression was that Ilya was running s390x the
> BPF selftests quite recently?

I have not investigated why on s390 it is zext'ed, but on x86 not,
it's related to the size of the register when it returns 32bit value.
There may be a bug there as well.

I did think a bit more on your words, making the zext patching code
specially check jumps and adjust the offset in the patchlet looks more
correct. But duplicates the existing code. I should spend more time on
that.


>
> > The problem was triggered by bpf selftest
> >
> > test_progs -t global_funcs
> >
> > on s390, where the very first "call" instruction is patched from
> > verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.
> >
> > The patch includes current instruction to the condition check.
> >
> > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> > ---
> >   kernel/bpf/core.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index ed0b3578867c..b0a9a22491a5 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct bpf_insn *insn, u32 pos, s32 end_old,
> >       s32 delta = end_new - end_old;
> >       s64 imm = insn->imm;
> >
> > -     if (curr < pos && curr + imm + 1 >= end_old)
> > +     if (curr <= pos && curr + imm + 1 >= end_old)
> >               imm += delta;
> >       else if (curr >= end_new && curr + imm + 1 < end_new)
> >               imm -= delta;
> > @@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct bpf_insn *insn, u32 pos, s32 end_old,
> >       s32 delta = end_new - end_old;
> >       s32 off = insn->off;
> >
> > -     if (curr < pos && curr + off + 1 >= end_old)
> > +     if (curr <= pos && curr + off + 1 >= end_old)
> >               off += delta;
> >       else if (curr >= end_new && curr + off + 1 < end_new)
> >               off -= delta;
> >
>


-- 
WBR, Yauheni

Re: [PATCH RFC] bpf: update current instruction on patching

[Yauheni Kaliuta]

On Thu, Sep 3, 2020 at 7:13 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:

[...]
>
> I have not investigated why on s390 it is zext'ed, but on x86 not,
> it's related to the size of the register when it returns 32bit value.
> There may be a bug there as well.

Nevermind, I missed that for x86 it's for 32 bits only. So, expected.

[...]

-- 
WBR, Yauheni

Re: [PATCH RFC] bpf: update current instruction on patching

[Ilya Leoshkevich]

On Thu, 2020-09-03 at 17:10 +0200, Daniel Borkmann wrote:
> On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
> > On code patching it may require to update branch destinations if
> > the
> > code size changed. bpf_adj_delta_to_imm/off increments offset only
> > if the patched area is after the branch instruction. But it's
> > possible, that the patched area itself is a branch instruction and
> > requires destination update.
> 
> Could you provide a concrete example and walk us through? I'm
> probably
> missing something but if the patchlet contains a branch instruction,
> then
> it should be 'self-contained'. In the sense that the patchlet is a
> 'black
> box' that replaces 1 insns with n insns but there is no awareness
> what's
> inside these insns and hence no fixup for that inside
> bpf_patch_insn_data().
> So, if we take an existing branch insns from the code, move it into
> the
> patchlet and extend beginning or end, then it feels more like a bug
> to the
> one that called bpf_patch_insn_data(), aka zext code here. Bit
> puzzled why
> this is only seen now, my impression was that Ilya was running s390x
> the
> BPF selftests quite recently?
> 
> > The problem was triggered by bpf selftest
> > 
> > test_progs -t global_funcs
> > 
> > on s390, where the very first "call" instruction is patched from
> > verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.
> > 
> > The patch includes current instruction to the condition check.
> > 
> > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> > ---
> >   kernel/bpf/core.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index ed0b3578867c..b0a9a22491a5 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct bpf_insn
> > *insn, u32 pos, s32 end_old,
> >   	s32 delta = end_new - end_old;
> >   	s64 imm = insn->imm;
> >   
> > -	if (curr < pos && curr + imm + 1 >= end_old)
> > +	if (curr <= pos && curr + imm + 1 >= end_old)
> >   		imm += delta;
> >   	else if (curr >= end_new && curr + imm + 1 < end_new)
> >   		imm -= delta;
> > @@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct bpf_insn
> > *insn, u32 pos, s32 end_old,
> >   	s32 delta = end_new - end_old;
> >   	s32 off = insn->off;
> >   
> > -	if (curr < pos && curr + off + 1 >= end_old)
> > +	if (curr <= pos && curr + off + 1 >= end_old)
> >   		off += delta;
> >   	else if (curr >= end_new && curr + off + 1 < end_new)
> >   		off -= delta;
> > 

Hi!

Last time I ran selftests against bpf-next ~1 month ago, and I don't
remember seeing any test_progs failures. Now I tried it again, and I
see the same problem as Yauheni. So this must be relatively new - I'll
try to bisect the commit that caused this.

Best regards,
Ilya

Re: [PATCH RFC] bpf: update current instruction on patching

[Ilya Leoshkevich]

On Fri, 2020-09-04 at 17:17 +0200, Ilya Leoshkevich wrote:
> On Thu, 2020-09-03 at 17:10 +0200, Daniel Borkmann wrote:
> > On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
> > > On code patching it may require to update branch destinations if
> > > the
> > > code size changed. bpf_adj_delta_to_imm/off increments offset
> > > only
> > > if the patched area is after the branch instruction. But it's
> > > possible, that the patched area itself is a branch instruction
> > > and
> > > requires destination update.
> > 
> > Could you provide a concrete example and walk us through? I'm
> > probably
> > missing something but if the patchlet contains a branch
> > instruction,
> > then
> > it should be 'self-contained'. In the sense that the patchlet is a
> > 'black
> > box' that replaces 1 insns with n insns but there is no awareness
> > what's
> > inside these insns and hence no fixup for that inside
> > bpf_patch_insn_data().
> > So, if we take an existing branch insns from the code, move it into
> > the
> > patchlet and extend beginning or end, then it feels more like a bug
> > to the
> > one that called bpf_patch_insn_data(), aka zext code here. Bit
> > puzzled why
> > this is only seen now, my impression was that Ilya was running
> > s390x
> > the
> > BPF selftests quite recently?
> > 
> > > The problem was triggered by bpf selftest
> > > 
> > > test_progs -t global_funcs
> > > 
> > > on s390, where the very first "call" instruction is patched from
> > > verifier.c:opt_subreg_zext_lo32_rnd_hi32() with zext_patch.
> > > 
> > > The patch includes current instruction to the condition check.
> > > 
> > > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> > > ---
> > >   kernel/bpf/core.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > > index ed0b3578867c..b0a9a22491a5 100644
> > > --- a/kernel/bpf/core.c
> > > +++ b/kernel/bpf/core.c
> > > @@ -340,7 +340,7 @@ static int bpf_adj_delta_to_imm(struct
> > > bpf_insn
> > > *insn, u32 pos, s32 end_old,
> > >   	s32 delta = end_new - end_old;
> > >   	s64 imm = insn->imm;
> > >   
> > > -	if (curr < pos && curr + imm + 1 >= end_old)
> > > +	if (curr <= pos && curr + imm + 1 >= end_old)
> > >   		imm += delta;
> > >   	else if (curr >= end_new && curr + imm + 1 < end_new)
> > >   		imm -= delta;
> > > @@ -358,7 +358,7 @@ static int bpf_adj_delta_to_off(struct
> > > bpf_insn
> > > *insn, u32 pos, s32 end_old,
> > >   	s32 delta = end_new - end_old;
> > >   	s32 off = insn->off;
> > >   
> > > -	if (curr < pos && curr + off + 1 >= end_old)
> > > +	if (curr <= pos && curr + off + 1 >= end_old)
> > >   		off += delta;
> > >   	else if (curr >= end_new && curr + off + 1 < end_new)
> > >   		off -= delta;
> > > 
> 
> Hi!
> 
> Last time I ran selftests against bpf-next ~1 month ago, and I don't
> remember seeing any test_progs failures. Now I tried it again, and I
> see the same problem as Yauheni. So this must be relatively new -
> I'll
> try to bisect the commit that caused this.

Turns out it has been failing for quite some time. I stopped at

commit 23ad04669f81f958e9a4121b0266228d2eb3c357 (HEAD)
Author: Matteo Croce <mcroce@redhat.com>
Date:   Mon May 11 13:32:34 2020 +0200

    samples: bpf: Fix build error

because I can't build earlier commits on my new Debian 10 VM due to
linker errors. The asm in the failing test is quite simple, so it's
unlikely that LLVM ouput has changed in the meantime. So I must have
simply missed it. Sorry!

I'll double check whether there have been more unnoticed failures.

Re: [PATCH RFC] bpf: update current instruction on patching

[Ilya Leoshkevich]

On Thu, 2020-09-03 at 19:13 +0300, Yauheni Kaliuta wrote:
> On Thu, Sep 3, 2020 at 6:10 PM Daniel Borkmann <daniel@iogearbox.net>
> wrote:
> > On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
> > > On code patching it may require to update branch destinations if
> > > the
> > > code size changed. bpf_adj_delta_to_imm/off increments offset
> > > only
> > > if the patched area is after the branch instruction. But it's
> > > possible, that the patched area itself is a branch instruction
> > > and
> > > requires destination update.
> > 
> > Could you provide a concrete example and walk us through? I'm
> > probably
> > missing something but if the patchlet contains a branch
> > instruction, then
> > it should be 'self-contained'. In the sense that the patchlet is a
> > 'black
> > box' that replaces 1 insns with n insns but there is no awareness
> > what's
> > inside these insns and hence no fixup for that inside
> > bpf_patch_insn_data().
> 
> The code is
> Disassembly of section classifier/test:
> 
> 0000000000000000 test_cls:
>        0:       85 01 00 00 ff ff ff ff call -1
>                 0000000000000000:  R_BPF_64_32  f7
>        1:       95 00 00 00 00 00 00 00 exit
> 0000000000000000 f1:
>        0:       61 01 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
>        1:       95 00 00 00 00 00 00 00 exit
> [...]
> 00000000000000a8 f7:
>       21:       85 01 00 00 ff ff ff ff call -1
>                 00000000000000a8:  R_BPF_64_32  f6
>       22:       95 00 00 00 00 00 00 00 exit
> 
> Before the patching the bytecode is:
> 
> 00000000: 85 01 00 00 00 00 00 16 95 00 00 00 00 00 00 00
> 00000010: 61 01 00 00 00 00 00 00 95 00 00 00 00 00 00 00
> [...]
> 
> It becomes
> 
> 
> 00000000: 85 01 00 00 00 00 00 2b bc 00 00 00 00 00 00 01
> 00000010: 95 00 00 00 00 00 00 00 61 01 00 80 00 00 00 00
> 
> at the end, the 2b offset is incorrect.
> 
> With that zext patching the code "85 01 00 00 00 00 00 16" is
> replaced
> with "85 01 00 00 00 00 00 16 bc 00 00 00 00 00 00 01", 0x16 is not
> changed, but the real offset has changed.
> 
> > So, if we take an existing branch insns from the code, move it into
> > the
> > patchlet and extend beginning or end, then it feels more like a bug
> > to the
> > one that called bpf_patch_insn_data(), aka zext code here. Bit
> > puzzled why
> > this is only seen now, my impression was that Ilya was running
> > s390x the
> > BPF selftests quite recently?
> 
> I have not investigated why on s390 it is zext'ed, but on x86 not,
> it's related to the size of the register when it returns 32bit value.
> There may be a bug there as well.
> 
> I did think a bit more on your words, making the zext patching code
> specially check jumps and adjust the offset in the patchlet looks
> more
> correct. But duplicates the existing code. I should spend more time
> on
> that.

I guess copying the existing insn into the patchlet was introduced
because there is nothing like bpf_insert_insns()? I.e. we can replace
an existing insn with a patchlet, but cannot append anything to it.
Would introducing such function solve this problem?

Re: [PATCH RFC] bpf: update current instruction on patching

[Yauheni Kaliuta]

Hi!

On Mon, Sep 7, 2020 at 7:14 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> On Thu, 2020-09-03 at 19:13 +0300, Yauheni Kaliuta wrote:
> > On Thu, Sep 3, 2020 at 6:10 PM Daniel Borkmann <daniel@iogearbox.net>
> > wrote:
> > > On 9/3/20 4:05 PM, Yauheni Kaliuta wrote:
> > > > On code patching it may require to update branch destinations if
> > > > the
> > > > code size changed. bpf_adj_delta_to_imm/off increments offset
> > > > only
> > > > if the patched area is after the branch instruction. But it's
> > > > possible, that the patched area itself is a branch instruction
> > > > and
> > > > requires destination update.
> > >
> > > Could you provide a concrete example and walk us through? I'm
> > > probably
> > > missing something but if the patchlet contains a branch
> > > instruction, then
> > > it should be 'self-contained'. In the sense that the patchlet is a
> > > 'black
> > > box' that replaces 1 insns with n insns but there is no awareness
> > > what's
> > > inside these insns and hence no fixup for that inside
> > > bpf_patch_insn_data().
> >
> > The code is
> > Disassembly of section classifier/test:
> >
> > 0000000000000000 test_cls:
> >        0:       85 01 00 00 ff ff ff ff call -1
> >                 0000000000000000:  R_BPF_64_32  f7
> >        1:       95 00 00 00 00 00 00 00 exit
> > 0000000000000000 f1:
> >        0:       61 01 00 00 00 00 00 00 r0 = *(u32 *)(r1 + 0)
> >        1:       95 00 00 00 00 00 00 00 exit
> > [...]
> > 00000000000000a8 f7:
> >       21:       85 01 00 00 ff ff ff ff call -1
> >                 00000000000000a8:  R_BPF_64_32  f6
> >       22:       95 00 00 00 00 00 00 00 exit
> >
> > Before the patching the bytecode is:
> >
> > 00000000: 85 01 00 00 00 00 00 16 95 00 00 00 00 00 00 00
> > 00000010: 61 01 00 00 00 00 00 00 95 00 00 00 00 00 00 00
> > [...]
> >
> > It becomes
> >
> >
> > 00000000: 85 01 00 00 00 00 00 2b bc 00 00 00 00 00 00 01
> > 00000010: 95 00 00 00 00 00 00 00 61 01 00 80 00 00 00 00
> >
> > at the end, the 2b offset is incorrect.
> >
> > With that zext patching the code "85 01 00 00 00 00 00 16" is
> > replaced
> > with "85 01 00 00 00 00 00 16 bc 00 00 00 00 00 00 01", 0x16 is not
> > changed, but the real offset has changed.
> >
> > > So, if we take an existing branch insns from the code, move it into
> > > the
> > > patchlet and extend beginning or end, then it feels more like a bug
> > > to the
> > > one that called bpf_patch_insn_data(), aka zext code here. Bit
> > > puzzled why
> > > this is only seen now, my impression was that Ilya was running
> > > s390x the
> > > BPF selftests quite recently?
> >
> > I have not investigated why on s390 it is zext'ed, but on x86 not,
> > it's related to the size of the register when it returns 32bit value.
> > There may be a bug there as well.
> >
> > I did think a bit more on your words, making the zext patching code
> > specially check jumps and adjust the offset in the patchlet looks
> > more
> > correct. But duplicates the existing code. I should spend more time
> > on
> > that.
>
> I guess copying the existing insn into the patchlet was introduced
> because there is nothing like bpf_insert_insns()? I.e. we can replace
> an existing insn with a patchlet, but cannot append anything to it.
> Would introducing such function solve this problem?

Sounds as a plan!


-- 
WBR, Yauheni