[PATCH] bpf: work around BPF verifier issues with AND and 2-reg branch conditions

[PATCH] bpf: work around BPF verifier issues with AND and 2-reg branch conditions

[Kris Van Hees]

Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
---
 bpf/strchr.S | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/bpf/strchr.S b/bpf/strchr.S
index 8365d9ce..42c6430f 100644
--- a/bpf/strchr.S
+++ b/bpf/strchr.S
@@ -26,8 +26,10 @@
  *
  *     // xor the char with every byte;  a match results in NULL byte
  *     r4 = roundup(r6, 8);
- *     for (r3 = 0; r3 < r4; r3 += 8)
- *         ((uint64_t *)dst)[r3] ^= c;
+ *     do {
+ *         r4 -= 8;
+ *         ((uint64_t *)dst)[r4] ^= c;
+ *     } while (r4 > 8);
  *
  *     // put a safeguard in place, then look for that NULL byte
  *     dst[r6] = '\0';
@@ -72,24 +74,24 @@ dt_strchr :
 	call	BPF_FUNC_probe_read_str	/* r6 = bpf_probe_read_str(dst, STRSZ, src) */
 	mov	%r6, %r0
 
-	jsle	%r6, 0, .Lerror
+	jsle	%r6, 1, .Lerror
 
 	sub	%r6, 1			/* r6-- */
 
 	mov	%r4, %r6		/* r4 = roundup(r6, 8) */
 	add	%r4, 7
-	and	%r4, -8
+	rsh	%r4, 3
+	lsh	%r4, 3
 
 	ldxdw	%r1, [%fp+-16]
-	mov	%r3, 0
-.Lloop:					/* for (r3 = 0; r3 < r4; r3 += 8) */
+.Lloop:					/* do { */
+	sub	%r4, 8			/*	r4 -= 8; */
 	ldxdw	%r5, [%fp+-24]
-	add	%r5, %r3
+	add	%r5, %r4
 	ldxdw	%r0, [%r5+0]
-	xor	%r0, %r1		/* 	((uint64_t *)dst)[r3] ^= c; */
+	xor	%r0, %r1		/* 	((uint64_t *)dst)[r4] ^= c; */
 	stxdw	[%r5+0], %r0
-	add	%r3, 8
-	jlt	%r3, %r4, .Lloop
+	jgt	%r4, 7, .Lloop		/* } while (r4 > 7); */
 
 	ldxdw	%r2, [%fp+-24]
 	add	%r2, %r6
-- 
2.42.0

Re: [PATCH] bpf: work around BPF verifier issues with AND and 2-reg branch conditions

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
two nits...

On 4/30/24 10:27, Kris Van Hees wrote:
> Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> ---
>   bpf/strchr.S | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/bpf/strchr.S b/bpf/strchr.S
> index 8365d9ce..42c6430f 100644
> --- a/bpf/strchr.S
> +++ b/bpf/strchr.S
> @@ -26,8 +26,10 @@
>    *
>    *     // xor the char with every byte;  a match results in NULL byte
>    *     r4 = roundup(r6, 8);
> - *     for (r3 = 0; r3 < r4; r3 += 8)
> - *         ((uint64_t *)dst)[r3] ^= c;
> + *     do {
> + *         r4 -= 8;
> + *         ((uint64_t *)dst)[r4] ^= c;
> + *     } while (r4 > 8);

I think that is wrong, but in any case out of step with the actual 
code.  How about either s/8/7/ or (imho better but then needs change in 
corresponding code) s/>/>=/.

>    *
>    *     // put a safeguard in place, then look for that NULL byte
>    *     dst[r6] = '\0';
> @@ -72,24 +74,24 @@ dt_strchr :
>   	call	BPF_FUNC_probe_read_str	/* r6 = bpf_probe_read_str(dst, STRSZ, src) */
>   	mov	%r6, %r0
>   
> -	jsle	%r6, 0, .Lerror
> +	jsle	%r6, 1, .Lerror
>   
>   	sub	%r6, 1			/* r6-- */
>   
>   	mov	%r4, %r6		/* r4 = roundup(r6, 8) */
>   	add	%r4, 7
> -	and	%r4, -8
> +	rsh	%r4, 3
> +	lsh	%r4, 3

How about a comment on the << >> 3 thing so that some future maintainer 
more easily understands why this looks so funny.

>   	ldxdw	%r1, [%fp+-16]
> -	mov	%r3, 0
> -.Lloop:					/* for (r3 = 0; r3 < r4; r3 += 8) */
> +.Lloop:					/* do { */
> +	sub	%r4, 8			/*	r4 -= 8; */
>   	ldxdw	%r5, [%fp+-24]
> -	add	%r5, %r3
> +	add	%r5, %r4
>   	ldxdw	%r0, [%r5+0]
> -	xor	%r0, %r1		/* 	((uint64_t *)dst)[r3] ^= c; */
> +	xor	%r0, %r1		/* 	((uint64_t *)dst)[r4] ^= c; */
>   	stxdw	[%r5+0], %r0
> -	add	%r3, 8
> -	jlt	%r3, %r4, .Lloop
> +	jgt	%r4, 7, .Lloop		/* } while (r4 > 7); */
>   
>   	ldxdw	%r2, [%fp+-24]
>   	add	%r2, %r6

Re: [PATCH] bpf: work around BPF verifier issues with AND and 2-reg branch conditions

[Kris Van Hees]

On Tue, Apr 30, 2024 at 12:54:12PM -0400, Eugene Loh wrote:
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> two nits...
> 
> On 4/30/24 10:27, Kris Van Hees wrote:
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> >   bpf/strchr.S | 22 ++++++++++++----------
> >   1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/bpf/strchr.S b/bpf/strchr.S
> > index 8365d9ce..42c6430f 100644
> > --- a/bpf/strchr.S
> > +++ b/bpf/strchr.S
> > @@ -26,8 +26,10 @@
> >    *
> >    *     // xor the char with every byte;  a match results in NULL byte
> >    *     r4 = roundup(r6, 8);
> > - *     for (r3 = 0; r3 < r4; r3 += 8)
> > - *         ((uint64_t *)dst)[r3] ^= c;
> > + *     do {
> > + *         r4 -= 8;
> > + *         ((uint64_t *)dst)[r4] ^= c;
> > + *     } while (r4 > 8);
> 
> I think that is wrong, but in any case out of step with the actual code. 
> How about either s/8/7/ or (imho better but then needs change in
> corresponding code) s/>/>=/.

Sure, we can use >= 8.  I'll apply the patch with that change.

> >    *
> >    *     // put a safeguard in place, then look for that NULL byte
> >    *     dst[r6] = '\0';
> > @@ -72,24 +74,24 @@ dt_strchr :
> >   	call	BPF_FUNC_probe_read_str	/* r6 = bpf_probe_read_str(dst, STRSZ, src) */
> >   	mov	%r6, %r0
> > -	jsle	%r6, 0, .Lerror
> > +	jsle	%r6, 1, .Lerror
> >   	sub	%r6, 1			/* r6-- */
> >   	mov	%r4, %r6		/* r4 = roundup(r6, 8) */
> >   	add	%r4, 7
> > -	and	%r4, -8
> > +	rsh	%r4, 3
> > +	lsh	%r4, 3
> 
> How about a comment on the << >> 3 thing so that some future maintainer more
> easily understands why this looks so funny.

I think that the >>= followed by <<= is a common enough idiom to mask out the
lower 3 bits that it does not warrant an explicit commit, especially as part
of a roundup() implementation.

> >   	ldxdw	%r1, [%fp+-16]
> > -	mov	%r3, 0
> > -.Lloop:					/* for (r3 = 0; r3 < r4; r3 += 8) */
> > +.Lloop:					/* do { */
> > +	sub	%r4, 8			/*	r4 -= 8; */
> >   	ldxdw	%r5, [%fp+-24]
> > -	add	%r5, %r3
> > +	add	%r5, %r4
> >   	ldxdw	%r0, [%r5+0]
> > -	xor	%r0, %r1		/* 	((uint64_t *)dst)[r3] ^= c; */
> > +	xor	%r0, %r1		/* 	((uint64_t *)dst)[r4] ^= c; */
> >   	stxdw	[%r5+0], %r0
> > -	add	%r3, 8
> > -	jlt	%r3, %r4, .Lloop
> > +	jgt	%r4, 7, .Lloop		/* } while (r4 > 7); */
> >   	ldxdw	%r2, [%fp+-24]
> >   	add	%r2, %r6