Re: [PATCH bpf-next v8 3/3] selftests: bpf: crypto skcipher algo selftests

[Martin KaFai Lau <martin.lau@linux.dev>]

On 1/15/24 2:08 PM, Vadim Fedorenko wrote:
> +static void deinit_afalg(void)
> +{
> +	if (tfmfd)

The test should be (tfmfd != -1) ?

> +		close(tfmfd);
> +	if (opfd)

Same here.

> +		close(opfd);
> +}

[ ... ]

> +SEC("?fentry.s/bpf_fentry_test1")
> +__failure __msg("Unreleased reference")

The error message is not checked. Take a look at how other tests use RUN_TESTS.

> +int BPF_PROG(crypto_acquire)
> +{
> +	struct bpf_crypto_ctx *cctx;
> +	struct bpf_dynptr key = {};
> +	int err = 0;
> +
> +	status = 0;
> +
> +	bpf_dynptr_from_mem(crypto_key, sizeof(crypto_key), 0, &key);
> +	cctx = bpf_crypto_ctx_create("skcipher", "ecb(aes)", &key, 0, &err);
> +
> +	if (!cctx) {
> +		status = err;
> +		return 0;
> +	}
> +
> +	cctx = bpf_crypto_ctx_acquire(cctx);
> +	if (!cctx)
> +		return -EINVAL;
> +
> +	bpf_crypto_ctx_release(cctx);
> +
> +	return 0;
> +}
> +
> +SEC("tc")
> +int decrypt_sanity(struct __sk_buff *skb)
> +{
> +	struct __crypto_ctx_value *v;
> +	struct bpf_crypto_ctx *ctx;
> +	struct bpf_dynptr psrc, pdst, iv;
> +	int err;
> +
> +	err = skb_dynptr_validate(skb, &psrc);
> +	if (err < 0) {
> +		status = err;
> +		return TC_ACT_SHOT;
> +	}
> +
> +	v = crypto_ctx_value_lookup();
> +	if (!v) {
> +		status = -ENOENT;
> +		return TC_ACT_SHOT;
> +	}
> +
> +	ctx = v->ctx;
> +	if (!ctx) {
> +		status = -ENOENT;
> +		return TC_ACT_SHOT;
> +	}
> +
> +	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);

It would be nice to allow passing NULL as an optional "iv" arg. It could be a 
future improvement.

Overall lgtm. Please add a cover letter in v4 and also the benchmark test that 
was brought up a while back.

> +
> +	status = bpf_crypto_decrypt(ctx, &psrc, &pdst, &iv);
> +
> +	return TC_ACT_SHOT;
> +}
> +
> +SEC("tc")
> +int encrypt_sanity(struct __sk_buff *skb)
> +{
> +	struct __crypto_ctx_value *v;
> +	struct bpf_crypto_ctx *ctx;
> +	struct bpf_dynptr psrc, pdst, iv;
> +	int err;
> +
> +	status = 0;
> +
> +	err = skb_dynptr_validate(skb, &psrc);
> +	if (err < 0) {
> +		status = err;
> +		return TC_ACT_SHOT;
> +	}
> +
> +	v = crypto_ctx_value_lookup();
> +	if (!v) {
> +		status = -ENOENT;
> +		return TC_ACT_SHOT;
> +	}
> +
> +	ctx = v->ctx;
> +	if (!ctx) {
> +		status = -ENOENT;
> +		return TC_ACT_SHOT;
> +	}
> +
> +	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);
> +
> +	return TC_ACT_SHOT;
> +}
> +
> +char __license[] SEC("license") = "GPL";