Re: [PATCH 2/3] crypto: acomp - Add setparam interface

[Eric Biggers <ebiggers@kernel.org>]

On Wed, May 07, 2025 at 10:20:58AM +0800, Herbert Xu wrote:
> On Tue, May 06, 2025 at 05:01:27PM +0100, Cabiddu, Giovanni wrote:
> >
> > > diff --git a/crypto/acompress.c b/crypto/acompress.c
> > > index 6fdf0ff9f3c0..cf37243a2a3c 100644
> > > --- a/crypto/acompress.c
> > > +++ b/crypto/acompress.c
> > ...
> > > +int crypto_acomp_setparam(struct crypto_acomp *tfm, const u8 *param,
> > > +			  unsigned int len)
> > Is the intent here to use strings to identify parameters? In such case,
> > `len` should be called `value`.
> > Or, is `param` a pointer to a structure?
> 
> param is just an arbitrary buffer with a length.  It's up to each
> algorithm to put an interpretation on param.
> 
> But I would recommend going with the existing Crypto API norm of
> using rtnl serialisation.
> 
> For example the existing struct zcomp_params (for zstd) would then
> look like this under rtnl (copied from authenc):
> 
> 	struct rtattr *rta = (struct rtattr *)param;
> 	struct crypto_zstd_param {
> 		__le32 dictlen;
> 		__le32 level;
> 	};
> 
> 	struct crypto_zstd_param *zstd_param;
> 
> 	if (!RTA_OK(rta, keylen))
> 		return -EINVAL;
> 	if (rta->rta_type != CRYPTO_AUTHENC_ZSTD_PARAM)
> 		return -EINVAL;
> 
> 	if (RTA_PAYLOAD(rta) != sizeof(*param))
> 		return -EINVAL;
> 
> 	zstd_param = RTA_DATA(rta);
> 	dictlen = le32_to_cpu(zstd_param->dictlen);
> 	level = le32_to_cpu(zstd_param->level);
> 
> 	param += rta->rta_len;
> 	len -= rta->rta_len;
> 
> 	if (len < dictlen)
> 		return -EINVAL;
> 
> 	dict = param;

That sounds crazy.  There's no need to serialize and deserialize byte streams
just for different parts of the kernel to talk to each other.

I'm still skeptical about the whole idea of trying to force people to go through
the Crypto API for compression.  It just results in the adoption of all the bad
ideas from the Crypto API.

- Eric