Re: [PATCH 1/2] CryptoAPI: prepare for processing multiple buffers at a time

[Fruhwirth Clemens <clemens@endorphin.org>]

[-- Attachment #1: Type: text/plain, Size: 4498 bytes --]

On Fri, 2005-01-14 at 14:10 +0100, Michal Ludvig wrote:

> This patch extends crypto/cipher.c for offloading the whole chaining modes
> to e.g. hardware crypto accelerators. It is much faster to let the 
> hardware do all the chaining if it can do so.

Is there any connection to Evgeniy Polyakov's acrypto work? It appears,
that there are two project for one objective. Would be nice to see both
parties pulling on one string.

> +	void (*cia_ecb)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> +			size_t nbytes, int encdec, int inplace);
> +	void (*cia_cbc)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> +			size_t nbytes, int encdec, int inplace);
> +	void (*cia_cfb)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> +			size_t nbytes, int encdec, int inplace);
> +	void (*cia_ofb)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> +			size_t nbytes, int encdec, int inplace);
> +	void (*cia_ctr)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> +			size_t nbytes, int encdec, int inplace);

What's the use of adding mode specific functions to the tfm struct? And
why do they all have the same function type? For instance, the "iv" or
"inplace" argument is meaningless for ECB.

Have a look at
http://clemens.endorphin.org/patches/lrw/2-tweakable-cipher-interface.diff

This patch takes the following approach to handle the 
cipher mode/interface issue:

Every mode is associated with one or more interfaces. This interface is
either cit_encrypt, cit_encrypt_iv, or cit_encrypt_tweaks. How these
interfaces are associated with cipher modes, is handled in
crypto_init_cipher_flags. 

Except for CBC, every mode associates with just one interface. In CBC,
the CryptoAPI caller can use the IV interface to supply an IV, or use
the current tfm's IV by using cit_encrypt instead of cit_encrypt_iv.

I don't see a gain to through dozens of pointers into the tfm, as a tfm
is always assigned a single mode.
 
>  /*
>   * Generic encrypt/decrypt wrapper for ciphers, handles operations across
> @@ -47,22 +101,101 @@ static inline void xor_128(u8 *a, const 
>  static int crypt(struct crypto_tfm *tfm,
>  		 struct scatterlist *dst,
>  		 struct scatterlist *src,
> -                 unsigned int nbytes, cryptfn_t crfn,
> -                 procfn_t prfn, int enc, void *info)

Your patch heavily interferes with my cleanup patch for crypt(..). To
put it briefly, I consider crypt(..) a mess. The function definition of
crypto and the procfn_t function is just a patchwork of stuff, added
when needed. 

I've rewritten a generic scatterwalker, that's a generic replacement for
crypto, that can apply any processing function with arbitrary argument
length to the data associated with a set of scatterlists. I think this
function shouldn't be in crypto/ but in some more generic location, as I
think it could be useful for much more things. 

http://clemens.endorphin.org/patches/lrw/1-generic-scatterwalker.diff
is the generic scatterwalk patch. 

int scatterwalk_walker_generic(void (function)(void *priv, int length,
void **buflist), void *priv, int steps, int nsl, ...) 

"function" is applied to the scatterlist data. 
"priv" is a private data structure for bookkeeping. It's supplied to the
function as first parameter.
"steps" is the number of times function is called.
"nsl" is the number of scatterlists following.

After "nsl", the scatterlists follow in a tuple of data:
<struct scatterlist *, int steplength, int ioflag>

ECB, for example:
	...
struct ecb_process_priv priv = { 
	.tfm = tfm,
	.crfn = tfm->__crt_alg->cra_cipher.cia_decrypt,
};
int bsize = crypto_tfm_alg_blocksize(tfm);
scatterwalk_walker_generic(ecb_process_gw, 	// processing function
	&priv,		// private data
	nbytes/bsize,	// number of steps
	2, 		// number of scatterlists
	dst, bsize, 1, 	// first, ioflag set to output
	src, bsize, 0);	// second, ioflag set to input

..
static void ecb_process_gw(void *_priv, int nsg, void **buf) 
{
	struct ecb_process_priv *priv = (struct ecb_process_priv *)_priv;
	char *dst = buf[0];	// pointer to correctly kmapped and copied dst
	char *src = buf[1];	// pointer to correctly kmapped and copied src
	priv->crfn(crypto_tfm_ctx(priv->tfm), dst, src);
}

Well, I recognize that I'm somehow off-topic now. But, it demonstrates
clearly, why we should get rid of crypt(..) and replace it with
something more generic.

-- 
Fruhwirth Clemens <clemens@endorphin.org>  http://clemens.endorphin.org

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]