diff mbox series

[bpf-next] bpf: x86: Explicitly zero-extend rax after 32-bit cmpxchg

Message ID 20210215171208.1181305-1-jackmanb@google.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next] bpf: x86: Explicitly zero-extend rax after 32-bit cmpxchg | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers fail 1 blamed authors not CCed: yhs@fb.com; 16 maintainers not CCed: bp@alien8.de andrii@kernel.org linux-kselftest@vger.kernel.org yhs@fb.com mingo@redhat.com davem@davemloft.net x86@kernel.org hpa@zytor.com yoshfuji@linux-ipv6.org john.fastabend@gmail.com netdev@vger.kernel.org kpsingh@kernel.org tglx@linutronix.de songliubraving@fb.com shuah@kernel.org kafai@fb.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 84 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Brendan Jackman Feb. 15, 2021, 5:12 p.m. UTC 
As pointed out by Ilya and explained in the new comment, there's a
discrepancy between x86 and BPF CMPXCHG semantics: BPF always loads
the value from memory into r0, while x86 only does so when r0 and the
value in memory are different.

At first this might sound like pure semantics, but it makes a real
difference when the comparison is 32-bit, since the load will
zero-extend r0/rax.

The fix is to explicitly zero-extend rax after doing such a CMPXCHG.

Note that this doesn't generate totally optimal code: at one of
emit_atomic's callsites (where BPF_{AND,OR,XOR} | BPF_FETCH are
implemented), the new mov is superfluous because there's already a
mov generated afterwards that will zero-extend r0. We could avoid
this unnecessary mov by just moving the new logic outside of
emit_atomic. But I think it's simpler to keep emit_atomic as a unit
of correctness (it generates the correct x86 code for a certain set
of BPF instructions, no further knowledge is needed to use it
correctly).

Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 arch/x86/net/bpf_jit_comp.c                   | 10 +++++++
 .../selftests/bpf/verifier/atomic_cmpxchg.c   | 25 ++++++++++++++++++
 .../selftests/bpf/verifier/atomic_or.c        | 26 +++++++++++++++++++
 3 files changed, 61 insertions(+)


base-commit: 5e1d40b75ed85ecd76347273da17e5da195c3e96

Comments

KP Singh Feb. 15, 2021, 9:05 p.m. UTC | #1 
On Mon, Feb 15, 2021 at 6:12 PM Brendan Jackman <jackmanb@google.com> wrote:
>
> As pointed out by Ilya and explained in the new comment, there's a
> discrepancy between x86 and BPF CMPXCHG semantics: BPF always loads
> the value from memory into r0, while x86 only does so when r0 and the
> value in memory are different.
>
> At first this might sound like pure semantics, but it makes a real
> difference when the comparison is 32-bit, since the load will
> zero-extend r0/rax.
>
> The fix is to explicitly zero-extend rax after doing such a CMPXCHG.
>
> Note that this doesn't generate totally optimal code: at one of
> emit_atomic's callsites (where BPF_{AND,OR,XOR} | BPF_FETCH are

I think this should be okay and was also suggested by Alexei in:

https://lore.kernel.org/bpf/CAADnVQ+gnQED7WYAw7Vmm5=omngCKYXnmgU_NqPUfESBerH8gQ@mail.gmail.com/

> implemented), the new mov is superfluous because there's already a
> mov generated afterwards that will zero-extend r0. We could avoid
> this unnecessary mov by just moving the new logic outside of
> emit_atomic. But I think it's simpler to keep emit_atomic as a unit
> of correctness (it generates the correct x86 code for a certain set
> of BPF instructions, no further knowledge is needed to use it
> correctly).
>
> Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Thanks for fixing this!

Acked-by: KP Singh <kpsingh@kernel.org>
Daniel Borkmann Feb. 15, 2021, 10:20 p.m. UTC | #2 
On 2/15/21 6:12 PM, Brendan Jackman wrote:
> As pointed out by Ilya and explained in the new comment, there's a
> discrepancy between x86 and BPF CMPXCHG semantics: BPF always loads
> the value from memory into r0, while x86 only does so when r0 and the
> value in memory are different.
> 
> At first this might sound like pure semantics, but it makes a real
> difference when the comparison is 32-bit, since the load will
> zero-extend r0/rax.
> 
> The fix is to explicitly zero-extend rax after doing such a CMPXCHG.
> 
> Note that this doesn't generate totally optimal code: at one of
> emit_atomic's callsites (where BPF_{AND,OR,XOR} | BPF_FETCH are
> implemented), the new mov is superfluous because there's already a
> mov generated afterwards that will zero-extend r0. We could avoid
> this unnecessary mov by just moving the new logic outside of
> emit_atomic. But I think it's simpler to keep emit_atomic as a unit
> of correctness (it generates the correct x86 code for a certain set
> of BPF instructions, no further knowledge is needed to use it
> correctly).
> 
> Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   arch/x86/net/bpf_jit_comp.c                   | 10 +++++++
>   .../selftests/bpf/verifier/atomic_cmpxchg.c   | 25 ++++++++++++++++++
>   .../selftests/bpf/verifier/atomic_or.c        | 26 +++++++++++++++++++
>   3 files changed, 61 insertions(+)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 79e7a0ec1da5..7919d5c54164 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -834,6 +834,16 @@ static int emit_atomic(u8 **pprog, u8 atomic_op,
>   
>   	emit_insn_suffix(&prog, dst_reg, src_reg, off);
>   
> +	if (atomic_op == BPF_CMPXCHG && bpf_size == BPF_W) {
> +		/*
> +		 * BPF_CMPXCHG unconditionally loads into R0, which means it
> +		 * zero-extends 32-bit values. However x86 CMPXCHG doesn't do a
> +		 * load if the comparison is successful. Therefore zero-extend
> +		 * explicitly.
> +		 */
> +		emit_mov_reg(&prog, false, BPF_REG_0, BPF_REG_0);

How does the situation look on other archs when they need to implement this in future?
Mainly asking whether it would be better to instead to move this logic into the verifier
instead, so it'll be consistent across all archs.

> +	}
> +
>   	*pprog = prog;
>   	return 0;
>   }
> diff --git a/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c b/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
> index 2efd8bcf57a1..6e52dfc64415 100644
> --- a/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
> +++ b/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
> @@ -94,3 +94,28 @@
>   	.result = REJECT,
>   	.errstr = "invalid read from stack",
>   },
> +{
> +	"BPF_W cmpxchg should zero top 32 bits",
> +	.insns = {
> +		/* r0 = U64_MAX; */
> +		BPF_MOV64_IMM(BPF_REG_0, 0),
> +		BPF_ALU64_IMM(BPF_SUB, BPF_REG_0, 1),
> +		/* u64 val = r0; */
> +		BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
> +		/* r0 = (u32)atomic_cmpxchg((u32 *)&val, r0, 1); */
> +		BPF_MOV32_IMM(BPF_REG_1, 1),
> +		BPF_ATOMIC_OP(BPF_W, BPF_CMPXCHG, BPF_REG_10, BPF_REG_1, -8),
> +		/* r1 = 0x00000000FFFFFFFFull; */
> +		BPF_MOV64_IMM(BPF_REG_1, 1),
> +		BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 32),
> +		BPF_ALU64_IMM(BPF_SUB, BPF_REG_1, 1),
> +		/* if (r0 != r1) exit(1); */
> +		BPF_JMP_REG(BPF_JEQ, BPF_REG_0, BPF_REG_1, 2),
> +		BPF_MOV32_IMM(BPF_REG_0, 1),
> +		BPF_EXIT_INSN(),
> +		/* exit(0); */
> +		BPF_MOV32_IMM(BPF_REG_0, 0),
> +		BPF_EXIT_INSN(),
> +	},
> +	.result = ACCEPT,
> +},
> diff --git a/tools/testing/selftests/bpf/verifier/atomic_or.c b/tools/testing/selftests/bpf/verifier/atomic_or.c
> index 70f982e1f9f0..e0811eb11542 100644
> --- a/tools/testing/selftests/bpf/verifier/atomic_or.c
> +++ b/tools/testing/selftests/bpf/verifier/atomic_or.c
> @@ -75,3 +75,29 @@
>   	},
>   	.result = ACCEPT,
>   },
> +{
> +	"BPF_W atomic or should zero top 32 bits",
> +	.insns = {
> +		/* r1 = U64_MAX; */
> +		BPF_MOV64_IMM(BPF_REG_1, 0),
> +		BPF_ALU64_IMM(BPF_SUB, BPF_REG_1, 1),
> +		/* u64 val = r0; */
> +		BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
> +		/* r1 = (u32)atomic_sub((u32 *)&val, 1); */
> +		BPF_MOV32_IMM(BPF_REG_1, 2),
> +		BPF_ATOMIC_OP(BPF_W, BPF_OR | BPF_FETCH, BPF_REG_10, BPF_REG_1, -8),
> +		/* r2 = 0x00000000FFFFFFFF; */
> +		BPF_MOV64_IMM(BPF_REG_2, 1),
> +		BPF_ALU64_IMM(BPF_LSH, BPF_REG_2, 32),
> +		BPF_ALU64_IMM(BPF_SUB, BPF_REG_2, 1),
> +		/* if (r2 != r1) exit(1); */
> +		BPF_JMP_REG(BPF_JEQ, BPF_REG_2, BPF_REG_1, 2),
> +		/* BPF_MOV32_IMM(BPF_REG_0, 1), */
> +		BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
> +		BPF_EXIT_INSN(),
> +		/* exit(0); */
> +		BPF_MOV32_IMM(BPF_REG_0, 0),
> +		BPF_EXIT_INSN(),
> +	},
> +	.result = ACCEPT,
> +},
> 
> base-commit: 5e1d40b75ed85ecd76347273da17e5da195c3e96
>
Ilya Leoshkevich Feb. 15, 2021, 10:24 p.m. UTC | #3 
On Mon, 2021-02-15 at 23:20 +0100, Daniel Borkmann wrote:
> On 2/15/21 6:12 PM, Brendan Jackman wrote:
> > As pointed out by Ilya and explained in the new comment, there's a
> > discrepancy between x86 and BPF CMPXCHG semantics: BPF always loads
> > the value from memory into r0, while x86 only does so when r0 and
> > the
> > value in memory are different.
> > 
> > At first this might sound like pure semantics, but it makes a real
> > difference when the comparison is 32-bit, since the load will
> > zero-extend r0/rax.
> > 
> > The fix is to explicitly zero-extend rax after doing such a
> > CMPXCHG.
> > 
> > Note that this doesn't generate totally optimal code: at one of
> > emit_atomic's callsites (where BPF_{AND,OR,XOR} | BPF_FETCH are
> > implemented), the new mov is superfluous because there's already a
> > mov generated afterwards that will zero-extend r0. We could avoid
> > this unnecessary mov by just moving the new logic outside of
> > emit_atomic. But I think it's simpler to keep emit_atomic as a unit
> > of correctness (it generates the correct x86 code for a certain set
> > of BPF instructions, no further knowledge is needed to use it
> > correctly).
> > 
> > Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
> > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > ---
> >   arch/x86/net/bpf_jit_comp.c                   | 10 +++++++
> >   .../selftests/bpf/verifier/atomic_cmpxchg.c   | 25
> > ++++++++++++++++++
> >   .../selftests/bpf/verifier/atomic_or.c        | 26
> > +++++++++++++++++++
> >   3 files changed, 61 insertions(+)
> > 
> > diff --git a/arch/x86/net/bpf_jit_comp.c
> > b/arch/x86/net/bpf_jit_comp.c
> > index 79e7a0ec1da5..7919d5c54164 100644
> > --- a/arch/x86/net/bpf_jit_comp.c
> > +++ b/arch/x86/net/bpf_jit_comp.c
> > @@ -834,6 +834,16 @@ static int emit_atomic(u8 **pprog, u8
> > atomic_op,
> >   
> >         emit_insn_suffix(&prog, dst_reg, src_reg, off);
> >   
> > +       if (atomic_op == BPF_CMPXCHG && bpf_size == BPF_W) {
> > +               /*
> > +                * BPF_CMPXCHG unconditionally loads into R0, which
> > means it
> > +                * zero-extends 32-bit values. However x86 CMPXCHG
> > doesn't do a
> > +                * load if the comparison is successful. Therefore
> > zero-extend
> > +                * explicitly.
> > +                */
> > +               emit_mov_reg(&prog, false, BPF_REG_0, BPF_REG_0);
> 
> How does the situation look on other archs when they need to
> implement this in future?
> Mainly asking whether it would be better to instead to move this
> logic into the verifier
> instead, so it'll be consistent across all archs.

I have exactly the same check in my s390 wip patch.
So having a common solution would be great.

[...]
Daniel Borkmann Feb. 15, 2021, 10:35 p.m. UTC | #4 
On 2/15/21 11:24 PM, Ilya Leoshkevich wrote:
> On Mon, 2021-02-15 at 23:20 +0100, Daniel Borkmann wrote:
>> On 2/15/21 6:12 PM, Brendan Jackman wrote:
>>> As pointed out by Ilya and explained in the new comment, there's a
>>> discrepancy between x86 and BPF CMPXCHG semantics: BPF always loads
>>> the value from memory into r0, while x86 only does so when r0 and
>>> the
>>> value in memory are different.
>>>
>>> At first this might sound like pure semantics, but it makes a real
>>> difference when the comparison is 32-bit, since the load will
>>> zero-extend r0/rax.
>>>
>>> The fix is to explicitly zero-extend rax after doing such a
>>> CMPXCHG.
>>>
>>> Note that this doesn't generate totally optimal code: at one of
>>> emit_atomic's callsites (where BPF_{AND,OR,XOR} | BPF_FETCH are
>>> implemented), the new mov is superfluous because there's already a
>>> mov generated afterwards that will zero-extend r0. We could avoid
>>> this unnecessary mov by just moving the new logic outside of
>>> emit_atomic. But I think it's simpler to keep emit_atomic as a unit
>>> of correctness (it generates the correct x86 code for a certain set
>>> of BPF instructions, no further knowledge is needed to use it
>>> correctly).
>>>
>>> Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
>>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>>> ---
>>>    arch/x86/net/bpf_jit_comp.c                   | 10 +++++++
>>>    .../selftests/bpf/verifier/atomic_cmpxchg.c   | 25
>>> ++++++++++++++++++
>>>    .../selftests/bpf/verifier/atomic_or.c        | 26
>>> +++++++++++++++++++
>>>    3 files changed, 61 insertions(+)
>>>
>>> diff --git a/arch/x86/net/bpf_jit_comp.c
>>> b/arch/x86/net/bpf_jit_comp.c
>>> index 79e7a0ec1da5..7919d5c54164 100644
>>> --- a/arch/x86/net/bpf_jit_comp.c
>>> +++ b/arch/x86/net/bpf_jit_comp.c
>>> @@ -834,6 +834,16 @@ static int emit_atomic(u8 **pprog, u8
>>> atomic_op,
>>>    
>>>          emit_insn_suffix(&prog, dst_reg, src_reg, off);
>>>    
>>> +       if (atomic_op == BPF_CMPXCHG && bpf_size == BPF_W) {
>>> +               /*
>>> +                * BPF_CMPXCHG unconditionally loads into R0, which
>>> means it
>>> +                * zero-extends 32-bit values. However x86 CMPXCHG
>>> doesn't do a
>>> +                * load if the comparison is successful. Therefore
>>> zero-extend
>>> +                * explicitly.
>>> +                */
>>> +               emit_mov_reg(&prog, false, BPF_REG_0, BPF_REG_0);
>>
>> How does the situation look on other archs when they need to
>> implement this in future?
>> Mainly asking whether it would be better to instead to move this
>> logic into the verifier
>> instead, so it'll be consistent across all archs.
> 
> I have exactly the same check in my s390 wip patch.
> So having a common solution would be great.

We do rewrites for various cases like div/mod handling, perhaps would be
best to emit an explicit BPF_MOV32_REG(insn->dst_reg, insn->dst_reg) there,
see the fixup_bpf_calls().
Ilya Leoshkevich Feb. 15, 2021, 10:42 p.m. UTC | #5 
On Mon, 2021-02-15 at 23:35 +0100, Daniel Borkmann wrote:
> On 2/15/21 11:24 PM, Ilya Leoshkevich wrote:
> > On Mon, 2021-02-15 at 23:20 +0100, Daniel Borkmann wrote:
> > > On 2/15/21 6:12 PM, Brendan Jackman wrote:
> > > > As pointed out by Ilya and explained in the new comment,
> > > > there's a
> > > > discrepancy between x86 and BPF CMPXCHG semantics: BPF always
> > > > loads
> > > > the value from memory into r0, while x86 only does so when r0
> > > > and
> > > > the
> > > > value in memory are different.
> > > > 
> > > > At first this might sound like pure semantics, but it makes a
> > > > real
> > > > difference when the comparison is 32-bit, since the load will
> > > > zero-extend r0/rax.
> > > > 
> > > > The fix is to explicitly zero-extend rax after doing such a
> > > > CMPXCHG.
> > > > 
> > > > Note that this doesn't generate totally optimal code: at one of
> > > > emit_atomic's callsites (where BPF_{AND,OR,XOR} | BPF_FETCH are
> > > > implemented), the new mov is superfluous because there's
> > > > already a
> > > > mov generated afterwards that will zero-extend r0. We could
> > > > avoid
> > > > this unnecessary mov by just moving the new logic outside of
> > > > emit_atomic. But I think it's simpler to keep emit_atomic as a
> > > > unit
> > > > of correctness (it generates the correct x86 code for a certain
> > > > set
> > > > of BPF instructions, no further knowledge is needed to use it
> > > > correctly).
> > > > 
> > > > Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > > > Fixes: 5ffa25502b5a ("bpf: Add instructions for
> > > > atomic_[cmp]xchg")
> > > > Signed-off-by: Brendan Jackman <jackmanb@google.com>
> > > > ---
> > > >    arch/x86/net/bpf_jit_comp.c                   | 10 +++++++
> > > >    .../selftests/bpf/verifier/atomic_cmpxchg.c   | 25
> > > > ++++++++++++++++++
> > > >    .../selftests/bpf/verifier/atomic_or.c        | 26
> > > > +++++++++++++++++++
> > > >    3 files changed, 61 insertions(+)
> > > > 
> > > > diff --git a/arch/x86/net/bpf_jit_comp.c
> > > > b/arch/x86/net/bpf_jit_comp.c
> > > > index 79e7a0ec1da5..7919d5c54164 100644
> > > > --- a/arch/x86/net/bpf_jit_comp.c
> > > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > > @@ -834,6 +834,16 @@ static int emit_atomic(u8 **pprog, u8
> > > > atomic_op,
> > > >    
> > > >          emit_insn_suffix(&prog, dst_reg, src_reg, off);
> > > >    
> > > > +       if (atomic_op == BPF_CMPXCHG && bpf_size == BPF_W) {
> > > > +               /*
> > > > +                * BPF_CMPXCHG unconditionally loads into R0,
> > > > which
> > > > means it
> > > > +                * zero-extends 32-bit values. However x86
> > > > CMPXCHG
> > > > doesn't do a
> > > > +                * load if the comparison is successful.
> > > > Therefore
> > > > zero-extend
> > > > +                * explicitly.
> > > > +                */
> > > > +               emit_mov_reg(&prog, false, BPF_REG_0,
> > > > BPF_REG_0);
> > > 
> > > How does the situation look on other archs when they need to
> > > implement this in future?
> > > Mainly asking whether it would be better to instead to move this
> > > logic into the verifier
> > > instead, so it'll be consistent across all archs.
> > 
> > I have exactly the same check in my s390 wip patch.
> > So having a common solution would be great.
> 
> We do rewrites for various cases like div/mod handling, perhaps would
> be
> best to emit an explicit BPF_MOV32_REG(insn->dst_reg, insn->dst_reg)
> there,
> see the fixup_bpf_calls().

How about BPF_ZEXT_REG? Then arches that don't need this (I think
aarch64's instruction always zero-extends) can detect this using
insn_is_zext() and skip such insns.
KP Singh Feb. 15, 2021, 11:30 p.m. UTC | #6 
On Mon, Feb 15, 2021 at 11:42 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>

[...]

> > > >
> > > > How does the situation look on other archs when they need to
> > > > implement this in future?
> > > > Mainly asking whether it would be better to instead to move this
> > > > logic into the verifier
> > > > instead, so it'll be consistent across all archs.
> > >
> > > I have exactly the same check in my s390 wip patch.
> > > So having a common solution would be great.
> >
> > We do rewrites for various cases like div/mod handling, perhaps would
> > be
> > best to emit an explicit BPF_MOV32_REG(insn->dst_reg, insn->dst_reg)
> > there,
> > see the fixup_bpf_calls().

Agreed, this would be better.

>
> How about BPF_ZEXT_REG? Then arches that don't need this (I think
> aarch64's instruction always zero-extends) can detect this using
> insn_is_zext() and skip such insns.
>

+1
Daniel Borkmann Feb. 16, 2021, 12:43 a.m. UTC | #7 
On 2/16/21 12:30 AM, KP Singh wrote:
> On Mon, Feb 15, 2021 at 11:42 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>
> 
> [...]
> 
>>>>> How does the situation look on other archs when they need to
>>>>> implement this in future?
>>>>> Mainly asking whether it would be better to instead to move this
>>>>> logic into the verifier
>>>>> instead, so it'll be consistent across all archs.
>>>>
>>>> I have exactly the same check in my s390 wip patch.
>>>> So having a common solution would be great.
>>>
>>> We do rewrites for various cases like div/mod handling, perhaps would
>>> be
>>> best to emit an explicit BPF_MOV32_REG(insn->dst_reg, insn->dst_reg)
>>> there,
>>> see the fixup_bpf_calls().
> 
> Agreed, this would be better.
> 
>>
>> How about BPF_ZEXT_REG? Then arches that don't need this (I think
>> aarch64's instruction always zero-extends) can detect this using
>> insn_is_zext() and skip such insns.
>>
> 
> +1

That would be nicer indeed.
diff mbox series

Patch

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 79e7a0ec1da5..7919d5c54164 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -834,6 +834,16 @@  static int emit_atomic(u8 **pprog, u8 atomic_op,
 
 	emit_insn_suffix(&prog, dst_reg, src_reg, off);
 
+	if (atomic_op == BPF_CMPXCHG && bpf_size == BPF_W) {
+		/*
+		 * BPF_CMPXCHG unconditionally loads into R0, which means it
+		 * zero-extends 32-bit values. However x86 CMPXCHG doesn't do a
+		 * load if the comparison is successful. Therefore zero-extend
+		 * explicitly.
+		 */
+		emit_mov_reg(&prog, false, BPF_REG_0, BPF_REG_0);
+	}
+
 	*pprog = prog;
 	return 0;
 }
diff --git a/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c b/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
index 2efd8bcf57a1..6e52dfc64415 100644
--- a/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
+++ b/tools/testing/selftests/bpf/verifier/atomic_cmpxchg.c
@@ -94,3 +94,28 @@ 
 	.result = REJECT,
 	.errstr = "invalid read from stack",
 },
+{
+	"BPF_W cmpxchg should zero top 32 bits",
+	.insns = {
+		/* r0 = U64_MAX; */
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_ALU64_IMM(BPF_SUB, BPF_REG_0, 1),
+		/* u64 val = r0; */
+		BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_0, -8),
+		/* r0 = (u32)atomic_cmpxchg((u32 *)&val, r0, 1); */
+		BPF_MOV32_IMM(BPF_REG_1, 1),
+		BPF_ATOMIC_OP(BPF_W, BPF_CMPXCHG, BPF_REG_10, BPF_REG_1, -8),
+		/* r1 = 0x00000000FFFFFFFFull; */
+		BPF_MOV64_IMM(BPF_REG_1, 1),
+		BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 32),
+		BPF_ALU64_IMM(BPF_SUB, BPF_REG_1, 1),
+		/* if (r0 != r1) exit(1); */
+		BPF_JMP_REG(BPF_JEQ, BPF_REG_0, BPF_REG_1, 2),
+		BPF_MOV32_IMM(BPF_REG_0, 1),
+		BPF_EXIT_INSN(),
+		/* exit(0); */
+		BPF_MOV32_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},
diff --git a/tools/testing/selftests/bpf/verifier/atomic_or.c b/tools/testing/selftests/bpf/verifier/atomic_or.c
index 70f982e1f9f0..e0811eb11542 100644
--- a/tools/testing/selftests/bpf/verifier/atomic_or.c
+++ b/tools/testing/selftests/bpf/verifier/atomic_or.c
@@ -75,3 +75,29 @@ 
 	},
 	.result = ACCEPT,
 },
+{
+	"BPF_W atomic or should zero top 32 bits",
+	.insns = {
+		/* r1 = U64_MAX; */
+		BPF_MOV64_IMM(BPF_REG_1, 0),
+		BPF_ALU64_IMM(BPF_SUB, BPF_REG_1, 1),
+		/* u64 val = r0; */
+		BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+		/* r1 = (u32)atomic_sub((u32 *)&val, 1); */
+		BPF_MOV32_IMM(BPF_REG_1, 2),
+		BPF_ATOMIC_OP(BPF_W, BPF_OR | BPF_FETCH, BPF_REG_10, BPF_REG_1, -8),
+		/* r2 = 0x00000000FFFFFFFF; */
+		BPF_MOV64_IMM(BPF_REG_2, 1),
+		BPF_ALU64_IMM(BPF_LSH, BPF_REG_2, 32),
+		BPF_ALU64_IMM(BPF_SUB, BPF_REG_2, 1),
+		/* if (r2 != r1) exit(1); */
+		BPF_JMP_REG(BPF_JEQ, BPF_REG_2, BPF_REG_1, 2),
+		/* BPF_MOV32_IMM(BPF_REG_0, 1), */
+		BPF_MOV64_REG(BPF_REG_0, BPF_REG_1),
+		BPF_EXIT_INSN(),
+		/* exit(0); */
+		BPF_MOV32_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	},
+	.result = ACCEPT,
+},