diff mbox series

[v8,03/12] bpf: allow self-xor operation

Message ID 20210913181510.46058-4-stephen@networkplumber.org (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series Packet capture framework enhancements | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger Sept. 13, 2021, 6:15 p.m. UTC
When doing BPF filter program conversion, a common way
to zero a register in single instruction is:
     xor r7,r7
The BPF validator would not allow this because the value of
r7 was undefined. But after this operation it always zero.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/bpf/bpf_validate.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Ananyev, Konstantin Sept. 15, 2021, 10:55 a.m. UTC | #1
> When doing BPF filter program conversion, a common way
> to zero a register in single instruction is:
>      xor r7,r7
> The BPF validator would not allow this because the value of
> r7 was undefined. But after this operation it always zero.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/bpf/bpf_validate.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c
> index 7b1291b382e9..7647a7454dc2 100644
> --- a/lib/bpf/bpf_validate.c
> +++ b/lib/bpf/bpf_validate.c
> @@ -661,8 +661,12 @@ eval_alu(struct bpf_verifier *bvf, const struct ebpf_insn *ins)
> 
>  	op = BPF_OP(ins->code);
> 
> -	err = eval_defined((op != EBPF_MOV) ? rd : NULL,
> -			(op != BPF_NEG) ? &rs : NULL);
> +	/* Allow self-xor as way to zero register */
> +	if (op == BPF_XOR && ins->src_reg == ins->dst_reg)
> +		err = NULL;
> +	else
> +		err = eval_defined((op != EBPF_MOV) ? rd : NULL,
> +				   (op != BPF_NEG) ? &rs : NULL);

Two things:
- We probably need to check that this is instruction with source register (not imm value).
- rd value is not evaluated to zero, while it probably should
  (will help evaluator to better predict further values) 

So might be better to do something like:

/* Allow self-xor as way to zero register */
        if (op == BPF_XOR && BPF_SRC(ins->code) == BPF_X &&
                        ins->src_reg == ins->dst_reg) {
                eval_fill_imm(&rs, UINT64_MAX, 0);
                eval_fill_imm(rd, UINT64_MAX, 0);
        }

        err = eval_defined((op != EBPF_MOV) ? rd : NULL,
                           (op != BPF_NEG) ? &rs : NULL);
        if (err != NULL)
                return err;

...

Another thing - shouldn't that patch be treated like a fix (cc to stable, etc.)?

>  	if (err != NULL)
>  		return err;
> 
> --
> 2.30.2
diff mbox series

Patch

diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c
index 7b1291b382e9..7647a7454dc2 100644
--- a/lib/bpf/bpf_validate.c
+++ b/lib/bpf/bpf_validate.c
@@ -661,8 +661,12 @@  eval_alu(struct bpf_verifier *bvf, const struct ebpf_insn *ins)
 
 	op = BPF_OP(ins->code);
 
-	err = eval_defined((op != EBPF_MOV) ? rd : NULL,
-			(op != BPF_NEG) ? &rs : NULL);
+	/* Allow self-xor as way to zero register */
+	if (op == BPF_XOR && ins->src_reg == ins->dst_reg)
+		err = NULL;
+	else
+		err = eval_defined((op != EBPF_MOV) ? rd : NULL,
+				   (op != BPF_NEG) ? &rs : NULL);
 	if (err != NULL)
 		return err;