[PATCH bpf-next v2 0/4] bpf: make trusted args nullable

[PATCH bpf-next v2 0/4] bpf: make trusted args nullable

[Vadim Fedorenko]

Current verifier checks for the arg to be nullable after checking for
certain pointer types. It prevents programs to pass NULL to kfunc args
even if they are marked as nullable. This patchset adjusts verifier and
changes bpf crypto kfuncs to allow null for IV parameter which is
optional for some ciphers. Benchmark shows ~4% improvements when there
is no need to initialise 0-sized dynptr.

v2:
- adjust kdoc accordingly

Vadim Fedorenko (4):
  bpf: verifier: make kfuncs args nullalble
  bpf: crypto: make state and IV dynptr nullable
  selftests: bpf: crypto: use NULL instead of 0-sized dynptr
  selftests: bpf: crypto: adjust bench to use nullable IV

 kernel/bpf/crypto.c                           | 26 +++++++++----------
 kernel/bpf/verifier.c                         |  6 ++---
 .../selftests/bpf/progs/crypto_bench.c        | 10 +++----
 .../selftests/bpf/progs/crypto_sanity.c       | 16 +++---------
 4 files changed, 24 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH bpf-next v2 1/4] bpf: verifier: make kfuncs args nullalble

[Vadim Fedorenko]

Some arguments to kfuncs might be NULL in some cases. But currently it's
not possible to pass NULL to any BTF structures because the check for
the suffix is located after all type checks. Move it to earlier place
to allow nullable args.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 kernel/bpf/verifier.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 9e3aba08984e..ed67aed3c284 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11179,6 +11179,9 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
 	if (btf_is_prog_ctx_type(&env->log, meta->btf, t, resolve_prog_type(env->prog), argno))
 		return KF_ARG_PTR_TO_CTX;
 
+	if (is_kfunc_arg_nullable(meta->btf, &args[argno]) && register_is_null(reg))
+		return KF_ARG_PTR_TO_NULL;
+
 	if (is_kfunc_arg_alloc_obj(meta->btf, &args[argno]))
 		return KF_ARG_PTR_TO_ALLOC_BTF_ID;
 
@@ -11224,9 +11227,6 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
 	if (is_kfunc_arg_callback(env, meta->btf, &args[argno]))
 		return KF_ARG_PTR_TO_CALLBACK;
 
-	if (is_kfunc_arg_nullable(meta->btf, &args[argno]) && register_is_null(reg))
-		return KF_ARG_PTR_TO_NULL;
-
 	if (argno + 1 < nargs &&
 	    (is_kfunc_arg_mem_size(meta->btf, &args[argno + 1], &regs[regno + 1]) ||
 	     is_kfunc_arg_const_mem_size(meta->btf, &args[argno + 1], &regs[regno + 1])))
-- 
2.43.0

Re: [PATCH bpf-next v2 1/4] bpf: verifier: make kfuncs args nullalble

[Eduard Zingerman]

On Fri, 2024-05-10 at 05:28 -0700, Vadim Fedorenko wrote:
> Some arguments to kfuncs might be NULL in some cases. But currently it's
> not possible to pass NULL to any BTF structures because the check for
> the suffix is located after all type checks. Move it to earlier place
> to allow nullable args.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

>  kernel/bpf/verifier.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 9e3aba08984e..ed67aed3c284 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11179,6 +11179,9 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
>  	if (btf_is_prog_ctx_type(&env->log, meta->btf, t, resolve_prog_type(env->prog), argno))
>  		return KF_ARG_PTR_TO_CTX;
>  
> +	if (is_kfunc_arg_nullable(meta->btf, &args[argno]) && register_is_null(reg))
> +		return KF_ARG_PTR_TO_NULL;
> +

Nit: maybe move this above the KF_ARG_PTR_TO_CTX check as well?

[PATCH bpf-next v2 2/4] bpf: crypto: make state and IV dynptr nullable

[Vadim Fedorenko]

Some ciphers do not require state and IV buffer, but with current
implementation 0-sized dynptr is always needed. With adjustment to
verifier we can provide NULL instead of 0-sized dynptr. Make crypto
kfuncs ready for this.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 kernel/bpf/crypto.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/crypto.c b/kernel/bpf/crypto.c
index 2bee4af91e38..ca25ed32e1cb 100644
--- a/kernel/bpf/crypto.c
+++ b/kernel/bpf/crypto.c
@@ -275,7 +275,7 @@ static int bpf_crypto_crypt(const struct bpf_crypto_ctx *ctx,
 	if (__bpf_dynptr_is_rdonly(dst))
 		return -EINVAL;
 
-	siv_len = __bpf_dynptr_size(siv);
+	siv_len = siv ? __bpf_dynptr_size(siv) : 0;
 	src_len = __bpf_dynptr_size(src);
 	dst_len = __bpf_dynptr_size(dst);
 	if (!src_len || !dst_len)
@@ -303,36 +303,36 @@ static int bpf_crypto_crypt(const struct bpf_crypto_ctx *ctx,
 
 /**
  * bpf_crypto_decrypt() - Decrypt buffer using configured context and IV provided.
- * @ctx:	The crypto context being used. The ctx must be a trusted pointer.
- * @src:	bpf_dynptr to the encrypted data. Must be a trusted pointer.
- * @dst:	bpf_dynptr to the buffer where to store the result. Must be a trusted pointer.
- * @siv:	bpf_dynptr to IV data and state data to be used by decryptor.
+ * @ctx:		The crypto context being used. The ctx must be a trusted pointer.
+ * @src:		bpf_dynptr to the encrypted data. Must be a trusted pointer.
+ * @dst:		bpf_dynptr to buffer where to store the result. Must be a trusted pointer.
+ * @siv__nullable:	bpf_dynptr to IV data and state data to be used by decryptor. May be NULL.
  *
  * Decrypts provided buffer using IV data and the crypto context. Crypto context must be configured.
  */
 __bpf_kfunc int bpf_crypto_decrypt(struct bpf_crypto_ctx *ctx,
 				   const struct bpf_dynptr_kern *src,
 				   const struct bpf_dynptr_kern *dst,
-				   const struct bpf_dynptr_kern *siv)
+				   const struct bpf_dynptr_kern *siv__nullable)
 {
-	return bpf_crypto_crypt(ctx, src, dst, siv, true);
+	return bpf_crypto_crypt(ctx, src, dst, siv__nullable, true);
 }
 
 /**
  * bpf_crypto_encrypt() - Encrypt buffer using configured context and IV provided.
- * @ctx:	The crypto context being used. The ctx must be a trusted pointer.
- * @src:	bpf_dynptr to the plain data. Must be a trusted pointer.
- * @dst:	bpf_dynptr to buffer where to store the result. Must be a trusted pointer.
- * @siv:	bpf_dynptr to IV data and state data to be used by decryptor.
+ * @ctx:		The crypto context being used. The ctx must be a trusted pointer.
+ * @src:		bpf_dynptr to the plain data. Must be a trusted pointer.
+ * @dst:		bpf_dynptr to buffer where to store the result. Must be a trusted pointer.
+ * @siv__nullable:	bpf_dynptr to IV data and state data to be used by decryptor. May be NULL.
  *
  * Encrypts provided buffer using IV data and the crypto context. Crypto context must be configured.
  */
 __bpf_kfunc int bpf_crypto_encrypt(struct bpf_crypto_ctx *ctx,
 				   const struct bpf_dynptr_kern *src,
 				   const struct bpf_dynptr_kern *dst,
-				   const struct bpf_dynptr_kern *siv)
+				   const struct bpf_dynptr_kern *siv__nullable)
 {
-	return bpf_crypto_crypt(ctx, src, dst, siv, false);
+	return bpf_crypto_crypt(ctx, src, dst, siv__nullable, false);
 }
 
 __bpf_kfunc_end_defs();
-- 
2.43.0

Re: [PATCH bpf-next v2 2/4] bpf: crypto: make state and IV dynptr nullable

[Eduard Zingerman]

On Fri, 2024-05-10 at 05:28 -0700, Vadim Fedorenko wrote:
> Some ciphers do not require state and IV buffer, but with current
> implementation 0-sized dynptr is always needed. With adjustment to
> verifier we can provide NULL instead of 0-sized dynptr. Make crypto
> kfuncs ready for this.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[PATCH bpf-next v2 3/4] selftests: bpf: crypto: use NULL instead of 0-sized dynptr

[Vadim Fedorenko]

Adjust selftests to use nullable option for state and IV arg.

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 .../testing/selftests/bpf/progs/crypto_sanity.c  | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/crypto_sanity.c b/tools/testing/selftests/bpf/progs/crypto_sanity.c
index 1be0a3fa5efd..645be6cddf36 100644
--- a/tools/testing/selftests/bpf/progs/crypto_sanity.c
+++ b/tools/testing/selftests/bpf/progs/crypto_sanity.c
@@ -89,7 +89,7 @@ int decrypt_sanity(struct __sk_buff *skb)
 {
 	struct __crypto_ctx_value *v;
 	struct bpf_crypto_ctx *ctx;
-	struct bpf_dynptr psrc, pdst, iv;
+	struct bpf_dynptr psrc, pdst;
 	int err;
 
 	err = skb_dynptr_validate(skb, &psrc);
@@ -114,12 +114,8 @@ int decrypt_sanity(struct __sk_buff *skb)
 	 * production code, a percpu map should be used to store the result.
 	 */
 	bpf_dynptr_from_mem(dst, sizeof(dst), 0, &pdst);
-	/* iv dynptr has to be initialized with 0 size, but proper memory region
-	 * has to be provided anyway
-	 */
-	bpf_dynptr_from_mem(dst, 0, 0, &iv);
 
-	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, &iv);
+	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, NULL);
 
 	return TC_ACT_SHOT;
 }
@@ -129,7 +125,7 @@ int encrypt_sanity(struct __sk_buff *skb)
 {
 	struct __crypto_ctx_value *v;
 	struct bpf_crypto_ctx *ctx;
-	struct bpf_dynptr psrc, pdst, iv;
+	struct bpf_dynptr psrc, pdst;
 	int err;
 
 	status = 0;
@@ -156,12 +152,8 @@ int encrypt_sanity(struct __sk_buff *skb)
 	 * production code, a percpu map should be used to store the result.
 	 */
 	bpf_dynptr_from_mem(dst, sizeof(dst), 0, &pdst);
-	/* iv dynptr has to be initialized with 0 size, but proper memory region
-	 * has to be provided anyway
-	 */
-	bpf_dynptr_from_mem(dst, 0, 0, &iv);
 
-	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, &iv);
+	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, NULL);
 
 	return TC_ACT_SHOT;
 }
-- 
2.43.0

Re: [PATCH bpf-next v2 3/4] selftests: bpf: crypto: use NULL instead of 0-sized dynptr

[Eduard Zingerman]

On Fri, 2024-05-10 at 05:28 -0700, Vadim Fedorenko wrote:
> Adjust selftests to use nullable option for state and IV arg.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[PATCH bpf-next v2 4/4] selftests: bpf: crypto: adjust bench to use nullable IV

[Vadim Fedorenko]

The bench shows some improvements, around 4% faster on decrypt.

Before:

Benchmark 'crypto-decrypt' started.
Iter   0 (325.719us): hits    5.105M/s (  5.105M/prod), drops 0.000M/s, total operations    5.105M/s
Iter   1 (-17.295us): hits    5.224M/s (  5.224M/prod), drops 0.000M/s, total operations    5.224M/s
Iter   2 (  5.504us): hits    4.630M/s (  4.630M/prod), drops 0.000M/s, total operations    4.630M/s
Iter   3 (  9.239us): hits    5.148M/s (  5.148M/prod), drops 0.000M/s, total operations    5.148M/s
Iter   4 ( 37.885us): hits    5.198M/s (  5.198M/prod), drops 0.000M/s, total operations    5.198M/s
Iter   5 (-53.282us): hits    5.167M/s (  5.167M/prod), drops 0.000M/s, total operations    5.167M/s
Iter   6 (-17.809us): hits    5.186M/s (  5.186M/prod), drops 0.000M/s, total operations    5.186M/s
Summary: hits    5.092 ± 0.228M/s (  5.092M/prod), drops    0.000 ±0.000M/s, total operations    5.092 ± 0.228M/s

After:

Benchmark 'crypto-decrypt' started.
Iter   0 (268.912us): hits    5.312M/s (  5.312M/prod), drops 0.000M/s, total operations    5.312M/s
Iter   1 (124.869us): hits    5.354M/s (  5.354M/prod), drops 0.000M/s, total operations    5.354M/s
Iter   2 (-36.801us): hits    5.334M/s (  5.334M/prod), drops 0.000M/s, total operations    5.334M/s
Iter   3 (254.628us): hits    5.334M/s (  5.334M/prod), drops 0.000M/s, total operations    5.334M/s
Iter   4 (-77.691us): hits    5.275M/s (  5.275M/prod), drops 0.000M/s, total operations    5.275M/s
Iter   5 (-164.510us): hits    5.313M/s (  5.313M/prod), drops 0.000M/s, total operations    5.313M/s
Iter   6 (-81.376us): hits    5.346M/s (  5.346M/prod), drops 0.000M/s, total operations    5.346M/s
Summary: hits    5.326 ± 0.029M/s (  5.326M/prod), drops    0.000 ±0.000M/s, total operations    5.326 ± 0.029M/s

Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
---
 tools/testing/selftests/bpf/progs/crypto_bench.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/crypto_bench.c b/tools/testing/selftests/bpf/progs/crypto_bench.c
index e61fe0882293..4ac956b26240 100644
--- a/tools/testing/selftests/bpf/progs/crypto_bench.c
+++ b/tools/testing/selftests/bpf/progs/crypto_bench.c
@@ -57,7 +57,7 @@ int crypto_encrypt(struct __sk_buff *skb)
 {
 	struct __crypto_ctx_value *v;
 	struct bpf_crypto_ctx *ctx;
-	struct bpf_dynptr psrc, pdst, iv;
+	struct bpf_dynptr psrc, pdst;
 
 	v = crypto_ctx_value_lookup();
 	if (!v) {
@@ -73,9 +73,8 @@ int crypto_encrypt(struct __sk_buff *skb)
 
 	bpf_dynptr_from_skb(skb, 0, &psrc);
 	bpf_dynptr_from_mem(dst, len, 0, &pdst);
-	bpf_dynptr_from_mem(dst, 0, 0, &iv);
 
-	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, &iv);
+	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, NULL);
 	__sync_add_and_fetch(&hits, 1);
 
 	return 0;
@@ -84,7 +83,7 @@ int crypto_encrypt(struct __sk_buff *skb)
 SEC("tc")
 int crypto_decrypt(struct __sk_buff *skb)
 {
-	struct bpf_dynptr psrc, pdst, iv;
+	struct bpf_dynptr psrc, pdst;
 	struct __crypto_ctx_value *v;
 	struct bpf_crypto_ctx *ctx;
 
@@ -98,9 +97,8 @@ int crypto_decrypt(struct __sk_buff *skb)
 
 	bpf_dynptr_from_skb(skb, 0, &psrc);
 	bpf_dynptr_from_mem(dst, len, 0, &pdst);
-	bpf_dynptr_from_mem(dst, 0, 0, &iv);
 
-	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, &iv);
+	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, NULL);
 	__sync_add_and_fetch(&hits, 1);
 
 	return 0;
-- 
2.43.0

Re: [PATCH bpf-next v2 4/4] selftests: bpf: crypto: adjust bench to use nullable IV

[Eduard Zingerman]

On Fri, 2024-05-10 at 05:28 -0700, Vadim Fedorenko wrote:
> The bench shows some improvements, around 4% faster on decrypt.
> 
> Before:
> 
> Benchmark 'crypto-decrypt' started.
> Iter   0 (325.719us): hits    5.105M/s (  5.105M/prod), drops 0.000M/s, total operations    5.105M/s
> Iter   1 (-17.295us): hits    5.224M/s (  5.224M/prod), drops 0.000M/s, total operations    5.224M/s
> Iter   2 (  5.504us): hits    4.630M/s (  4.630M/prod), drops 0.000M/s, total operations    4.630M/s
> Iter   3 (  9.239us): hits    5.148M/s (  5.148M/prod), drops 0.000M/s, total operations    5.148M/s
> Iter   4 ( 37.885us): hits    5.198M/s (  5.198M/prod), drops 0.000M/s, total operations    5.198M/s
> Iter   5 (-53.282us): hits    5.167M/s (  5.167M/prod), drops 0.000M/s, total operations    5.167M/s
> Iter   6 (-17.809us): hits    5.186M/s (  5.186M/prod), drops 0.000M/s, total operations    5.186M/s
> Summary: hits    5.092 ± 0.228M/s (  5.092M/prod), drops    0.000 ±0.000M/s, total operations    5.092 ± 0.228M/s
> 
> After:
> 
> Benchmark 'crypto-decrypt' started.
> Iter   0 (268.912us): hits    5.312M/s (  5.312M/prod), drops 0.000M/s, total operations    5.312M/s
> Iter   1 (124.869us): hits    5.354M/s (  5.354M/prod), drops 0.000M/s, total operations    5.354M/s
> Iter   2 (-36.801us): hits    5.334M/s (  5.334M/prod), drops 0.000M/s, total operations    5.334M/s
> Iter   3 (254.628us): hits    5.334M/s (  5.334M/prod), drops 0.000M/s, total operations    5.334M/s
> Iter   4 (-77.691us): hits    5.275M/s (  5.275M/prod), drops 0.000M/s, total operations    5.275M/s
> Iter   5 (-164.510us): hits    5.313M/s (  5.313M/prod), drops 0.000M/s, total operations    5.313M/s
> Iter   6 (-81.376us): hits    5.346M/s (  5.346M/prod), drops 0.000M/s, total operations    5.346M/s
> Summary: hits    5.326 ± 0.029M/s (  5.326M/prod), drops    0.000 ±0.000M/s, total operations    5.326 ± 0.029M/s
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Re: [PATCH bpf-next v2 4/4] selftests: bpf: crypto: adjust bench to use nullable IV

[Martin KaFai Lau]

On 5/10/24 5:28 AM, Vadim Fedorenko wrote:
> The bench shows some improvements, around 4% faster on decrypt.

The original intention is to make the crypto kfunc more ergonomic to use such 
that the bpf prog does not have to initialize a zero length dynptr for the 
optional dynptr argument.

This performance boost is a decent surprise considering the crypto operation 
should be pretty heavy. (thanks for having the crypto benchmark handy).

Do you have a chance to get a perf record to confirm where the cycles is saved?

Why it only helps decrypt?

Inlining it would be nice (as Eduard mentioned in another thread). I also wonder 
if Eduard's work on the no caller saved registers could help the dynptr kfunc? I 
think the dynptr kfunc optimization could be a followup.

> 
> Before:
> 
> Benchmark 'crypto-decrypt' started.
> Iter   0 (325.719us): hits    5.105M/s (  5.105M/prod), drops 0.000M/s, total operations    5.105M/s
> Iter   1 (-17.295us): hits    5.224M/s (  5.224M/prod), drops 0.000M/s, total operations    5.224M/s
> Iter   2 (  5.504us): hits    4.630M/s (  4.630M/prod), drops 0.000M/s, total operations    4.630M/s
> Iter   3 (  9.239us): hits    5.148M/s (  5.148M/prod), drops 0.000M/s, total operations    5.148M/s
> Iter   4 ( 37.885us): hits    5.198M/s (  5.198M/prod), drops 0.000M/s, total operations    5.198M/s
> Iter   5 (-53.282us): hits    5.167M/s (  5.167M/prod), drops 0.000M/s, total operations    5.167M/s
> Iter   6 (-17.809us): hits    5.186M/s (  5.186M/prod), drops 0.000M/s, total operations    5.186M/s
> Summary: hits    5.092 ± 0.228M/s (  5.092M/prod), drops    0.000 ±0.000M/s, total operations    5.092 ± 0.228M/s
> 
> After:
> 
> Benchmark 'crypto-decrypt' started.
> Iter   0 (268.912us): hits    5.312M/s (  5.312M/prod), drops 0.000M/s, total operations    5.312M/s
> Iter   1 (124.869us): hits    5.354M/s (  5.354M/prod), drops 0.000M/s, total operations    5.354M/s
> Iter   2 (-36.801us): hits    5.334M/s (  5.334M/prod), drops 0.000M/s, total operations    5.334M/s
> Iter   3 (254.628us): hits    5.334M/s (  5.334M/prod), drops 0.000M/s, total operations    5.334M/s
> Iter   4 (-77.691us): hits    5.275M/s (  5.275M/prod), drops 0.000M/s, total operations    5.275M/s
> Iter   5 (-164.510us): hits    5.313M/s (  5.313M/prod), drops 0.000M/s, total operations    5.313M/s
> Iter   6 (-81.376us): hits    5.346M/s (  5.346M/prod), drops 0.000M/s, total operations    5.346M/s
> Summary: hits    5.326 ± 0.029M/s (  5.326M/prod), drops    0.000 ±0.000M/s, total operations    5.326 ± 0.029M/s
> 
> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
> ---
>   tools/testing/selftests/bpf/progs/crypto_bench.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/crypto_bench.c b/tools/testing/selftests/bpf/progs/crypto_bench.c
> index e61fe0882293..4ac956b26240 100644
> --- a/tools/testing/selftests/bpf/progs/crypto_bench.c
> +++ b/tools/testing/selftests/bpf/progs/crypto_bench.c
> @@ -57,7 +57,7 @@ int crypto_encrypt(struct __sk_buff *skb)
>   {
>   	struct __crypto_ctx_value *v;
>   	struct bpf_crypto_ctx *ctx;
> -	struct bpf_dynptr psrc, pdst, iv;
> +	struct bpf_dynptr psrc, pdst;
>   
>   	v = crypto_ctx_value_lookup();
>   	if (!v) {
> @@ -73,9 +73,8 @@ int crypto_encrypt(struct __sk_buff *skb)
>   
>   	bpf_dynptr_from_skb(skb, 0, &psrc);
>   	bpf_dynptr_from_mem(dst, len, 0, &pdst);
> -	bpf_dynptr_from_mem(dst, 0, 0, &iv);
>   
> -	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, &iv);
> +	status = bpf_crypto_encrypt(ctx, &psrc, &pdst, NULL);
>   	__sync_add_and_fetch(&hits, 1);
>   
>   	return 0;
> @@ -84,7 +83,7 @@ int crypto_encrypt(struct __sk_buff *skb)
>   SEC("tc")
>   int crypto_decrypt(struct __sk_buff *skb)
>   {
> -	struct bpf_dynptr psrc, pdst, iv;
> +	struct bpf_dynptr psrc, pdst;
>   	struct __crypto_ctx_value *v;
>   	struct bpf_crypto_ctx *ctx;
>   
> @@ -98,9 +97,8 @@ int crypto_decrypt(struct __sk_buff *skb)
>   
>   	bpf_dynptr_from_skb(skb, 0, &psrc);
>   	bpf_dynptr_from_mem(dst, len, 0, &pdst);
> -	bpf_dynptr_from_mem(dst, 0, 0, &iv);
>   
> -	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, &iv);
> +	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, NULL);
>   	__sync_add_and_fetch(&hits, 1);
>   
>   	return 0;

Re: [PATCH bpf-next v2 4/4] selftests: bpf: crypto: adjust bench to use nullable IV

[Eduard Zingerman]

On Wed, 2024-05-22 at 11:01 -0700, Martin KaFai Lau wrote:


[...]

> Inlining it would be nice (as Eduard mentioned in another thread). I also wonder 
> if Eduard's work on the no caller saved registers could help the dynptr kfunc? I 
> think the dynptr kfunc optimization could be a followup.

For the context:
https://clang.llvm.org/docs/AttributeReference.html#no-caller-saved-registers

Basically the attribute says that compiler does not need
to save all r0-r5 registers for some function calls.
My changes for LLVM/verifier are not public yet,
I'll try to speedup.

Re: [PATCH bpf-next v2 4/4] selftests: bpf: crypto: adjust bench to use nullable IV

[Vadim Fedorenko]

On 22/05/2024 19:01, Martin KaFai Lau wrote:
> On 5/10/24 5:28 AM, Vadim Fedorenko wrote:
>> The bench shows some improvements, around 4% faster on decrypt.
> 
> The original intention is to make the crypto kfunc more ergonomic to use 
> such that the bpf prog does not have to initialize a zero length dynptr 
> for the optional dynptr argument.
> 
> This performance boost is a decent surprise considering the crypto 
> operation should be pretty heavy. (thanks for having the crypto 
> benchmark handy).
> 
> Do you have a chance to get a perf record to confirm where the cycles is 
> saved?

Not really, but it's just initialization part changed, I was using the
same base commit to do the comparison.

> 
> Why it only helps decrypt?

It helps both, I just didn't show encrypt output, but it's the same 4%

> 
> Inlining it would be nice (as Eduard mentioned in another thread). I 
> also wonder if Eduard's work on the no caller saved registers could help 
> the dynptr kfunc? I think the dynptr kfunc optimization could be a 
> followup.
> 
>>
>> Before:
>>
>> Benchmark 'crypto-decrypt' started.
>> Iter   0 (325.719us): hits    5.105M/s (  5.105M/prod), drops 
>> 0.000M/s, total operations    5.105M/s
>> Iter   1 (-17.295us): hits    5.224M/s (  5.224M/prod), drops 
>> 0.000M/s, total operations    5.224M/s
>> Iter   2 (  5.504us): hits    4.630M/s (  4.630M/prod), drops 
>> 0.000M/s, total operations    4.630M/s
>> Iter   3 (  9.239us): hits    5.148M/s (  5.148M/prod), drops 
>> 0.000M/s, total operations    5.148M/s
>> Iter   4 ( 37.885us): hits    5.198M/s (  5.198M/prod), drops 
>> 0.000M/s, total operations    5.198M/s
>> Iter   5 (-53.282us): hits    5.167M/s (  5.167M/prod), drops 
>> 0.000M/s, total operations    5.167M/s
>> Iter   6 (-17.809us): hits    5.186M/s (  5.186M/prod), drops 
>> 0.000M/s, total operations    5.186M/s
>> Summary: hits    5.092 ± 0.228M/s (  5.092M/prod), drops    0.000 
>> ±0.000M/s, total operations    5.092 ± 0.228M/s
>>
>> After:
>>
>> Benchmark 'crypto-decrypt' started.
>> Iter   0 (268.912us): hits    5.312M/s (  5.312M/prod), drops 
>> 0.000M/s, total operations    5.312M/s
>> Iter   1 (124.869us): hits    5.354M/s (  5.354M/prod), drops 
>> 0.000M/s, total operations    5.354M/s
>> Iter   2 (-36.801us): hits    5.334M/s (  5.334M/prod), drops 
>> 0.000M/s, total operations    5.334M/s
>> Iter   3 (254.628us): hits    5.334M/s (  5.334M/prod), drops 
>> 0.000M/s, total operations    5.334M/s
>> Iter   4 (-77.691us): hits    5.275M/s (  5.275M/prod), drops 
>> 0.000M/s, total operations    5.275M/s
>> Iter   5 (-164.510us): hits    5.313M/s (  5.313M/prod), drops 
>> 0.000M/s, total operations    5.313M/s
>> Iter   6 (-81.376us): hits    5.346M/s (  5.346M/prod), drops 
>> 0.000M/s, total operations    5.346M/s
>> Summary: hits    5.326 ± 0.029M/s (  5.326M/prod), drops    0.000 
>> ±0.000M/s, total operations    5.326 ± 0.029M/s
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>>   tools/testing/selftests/bpf/progs/crypto_bench.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/progs/crypto_bench.c 
>> b/tools/testing/selftests/bpf/progs/crypto_bench.c
>> index e61fe0882293..4ac956b26240 100644
>> --- a/tools/testing/selftests/bpf/progs/crypto_bench.c
>> +++ b/tools/testing/selftests/bpf/progs/crypto_bench.c
>> @@ -57,7 +57,7 @@ int crypto_encrypt(struct __sk_buff *skb)
>>   {
>>       struct __crypto_ctx_value *v;
>>       struct bpf_crypto_ctx *ctx;
>> -    struct bpf_dynptr psrc, pdst, iv;
>> +    struct bpf_dynptr psrc, pdst;
>>       v = crypto_ctx_value_lookup();
>>       if (!v) {
>> @@ -73,9 +73,8 @@ int crypto_encrypt(struct __sk_buff *skb)
>>       bpf_dynptr_from_skb(skb, 0, &psrc);
>>       bpf_dynptr_from_mem(dst, len, 0, &pdst);
>> -    bpf_dynptr_from_mem(dst, 0, 0, &iv);
>> -    status = bpf_crypto_encrypt(ctx, &psrc, &pdst, &iv);
>> +    status = bpf_crypto_encrypt(ctx, &psrc, &pdst, NULL);
>>       __sync_add_and_fetch(&hits, 1);
>>       return 0;
>> @@ -84,7 +83,7 @@ int crypto_encrypt(struct __sk_buff *skb)
>>   SEC("tc")
>>   int crypto_decrypt(struct __sk_buff *skb)
>>   {
>> -    struct bpf_dynptr psrc, pdst, iv;
>> +    struct bpf_dynptr psrc, pdst;
>>       struct __crypto_ctx_value *v;
>>       struct bpf_crypto_ctx *ctx;
>> @@ -98,9 +97,8 @@ int crypto_decrypt(struct __sk_buff *skb)
>>       bpf_dynptr_from_skb(skb, 0, &psrc);
>>       bpf_dynptr_from_mem(dst, len, 0, &pdst);
>> -    bpf_dynptr_from_mem(dst, 0, 0, &iv);
>> -    status = bpf_crypto_decrypt(ctx, &psrc, &pdst, &iv);
>> +    status = bpf_crypto_decrypt(ctx, &psrc, &pdst, NULL);
>>       __sync_add_and_fetch(&hits, 1);
>>       return 0;
> 

Re: [PATCH bpf-next v2 0/4] bpf: make trusted args nullable

[Eduard Zingerman]

On Fri, 2024-05-10 at 05:28 -0700, Vadim Fedorenko wrote:
> Current verifier checks for the arg to be nullable after checking for
> certain pointer types. It prevents programs to pass NULL to kfunc args
> even if they are marked as nullable. This patchset adjusts verifier and
> changes bpf crypto kfuncs to allow null for IV parameter which is
> optional for some ciphers. Benchmark shows ~4% improvements when there
> is no need to initialise 0-sized dynptr.
> 
> v2:
> - adjust kdoc accordingly
> 

Hi Vadim, sorry for late response.

I think that this patch-set looks good,
but I'd like to ask you to add a dedicated test to the following file:
tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c.
(crypto tests are for crypto and might be modified in the future
 w/o consideration of verifier pathways they currently test).

Also nullable dynptr sounds kind-of funny.
As far as I understand, performance gains are due to omission of extra
function call. Did you consider inlining for bpf_dynptr_from_mem()?
Same way it is done for bpf_kptr_xchg() in verifier.c:do_misc_fixups().

Thanks,
Eduard

Re: [PATCH bpf-next v2 0/4] bpf: make trusted args nullable

[Vadim Fedorenko]

On 22/05/2024 16:23, Eduard Zingerman wrote:
> On Wed, 2024-05-22 at 13:35 +0100, Vadim Fedorenko wrote:
> 
> [...]
> 
>> I'm not really sure how I can test it without fully replicating crypto
>> tests. We don't have any other kfuncs with nullable dynptrs yet to test,
>> maybe we should revisit this part once we have more functions with
>> nullable parameters.
> 
> kfuncs for testing could be defined in
> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:bpf_testmod.c,
> e.g. see bpf_testmod_test_mod_kfunc().

alright, thanks for the pointer, I'll add some tests with kfuncs.

Re: [PATCH bpf-next v2 0/4] bpf: make trusted args nullable

[Martin KaFai Lau]

On 5/22/24 9:34 AM, Vadim Fedorenko wrote:
> On 22/05/2024 16:23, Eduard Zingerman wrote:
>> On Wed, 2024-05-22 at 13:35 +0100, Vadim Fedorenko wrote:
>>
>> [...]
>>
>>> I'm not really sure how I can test it without fully replicating crypto
>>> tests. We don't have any other kfuncs with nullable dynptrs yet to test,
>>> maybe we should revisit this part once we have more functions with
>>> nullable parameters.
>>
>> kfuncs for testing could be defined in
>> tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c:bpf_testmod.c,
>> e.g. see bpf_testmod_test_mod_kfunc().
> 
> alright, thanks for the pointer, I'll add some tests with kfuncs.
> 
+1

pw-bot: cr