Hi Herbert, I had looked into what you suggested. What about this implementation design below. I will show an implementation for ahash type only: File: algapi.h struct crypto_type { unsigned int (*ctxsize)(struct crypto_alg *alg, u32 type, u32 mask); int (*init)(struct crypto_tfm *tfm, u32 type, u32 mask); void (*exit)(struct crypto_tfm *tfm); void (*show)(struct seq_file *m, struct crypto_alg *alg); int (*uspace_setparam)(struct crypto_tfm *tfm, void *param, u32 param_size); int (*uspace_op)(struct crypto_tfm *tfm, const struct iovec *iov, u32 iov_cnt, crypto_completion_t cb, void *ctx); }; Please note the extra function uspace_setparam and uspace_op. Please note that the parameter 'void *param' from function uspace_setparam are kernel space pointer. The parameter 'const struct iovec *iov' are kernel space pointer. The handling of user space pointers will be handled by cryptodev driver. Linux CryptoAPI algorithm type will always expect kernel accessible pointers. File: cryptodev.h struct uspace_session { __u8 alg_name[64]; __u32 front_type; /* I don't believe we need this - leave for now */ __u32 type; /* We might need to allow a value of 0 for any */ __u32 mask; /* We might need to allow a value of 0 for any */ }; struct uspace_param { __u16 size; /* We need this parameter as I/O control is only one pointer */ __u8 data[0]; /* Variable size - parameters are crypto type specific */ }; struct uspace_param_ahash { /* This is the data field of struct uspace_param */ __u16 hmackey_size; __u8 data[0]; }; struct uspace_param_aead { /* This is the data field of struct uspace_param */ __u16 key_size; __u16 hmackey_size; __u8 data[0]; }; /* Create crypto session */ #define CIOCGSESSION _IOWR('c', 101, struct session_op) /* Set algorithm parameters */ #define CIOCPARAM _IOWR('c', 102, struct key_op) struct upsace_aead_op { /* Per operation for aead type */ #define COP_NONE 0 #define COP_ENCRYPT 1 #define COP_DECRYPT 2 __u16 op; /* i.e. COP_ENCRYPT */ __u16 flags; __u16 iv_size; __u16 assoc_size; __u8 data[0]; /* parameter for aead per operation */ }; File: cryptodev.c Ioctl function: switch (cmd) { case CIOCGSESSION: a bunch of cryptodev code as before to decode alg_name, alg_type, alg_mask... /* Now create the session */ ca = __crypto_alg_lookup(alg_name, alg_type, alg_mask); tfm = __crypto_alloc_tfm(ca, alg_type, alg_mask); /* Now save ca and tfm into cryptodev context structure */ /* With this implementation, it relies on Linux CryptoAPI to create the tfm and no extra switch, if, and etc statement. */ break; case CIOCPARAM: a bunch of cryptodev code as before to decode 'struct uspace_param' /* Extract the alg type and tfm */ tftm = ... ca = ... ca->cra_type->uspace_setparam(tfm, param, param_size); break; } Write function (Per a operation): Cryptodev_write(...) { A bound of cryptodev code as before to map in the vector buffers. For ahash, it is just the data to be hashed. For aead, there is The data buffer and aead parameters. Plus cryptodev house works /* At this point, we have two pointers accessible by kernel - data to be hashed and result pointer - let call this iov[2] */ /* Extract tftm and crypto alg */ ca = ... tftm = ... ca->cra_type->uspace_op(tftm, iov, 2, cd_write_cb, context_ptr); return; } ... cd_write_cb(...) { A bound of cryptodev code as before... If pages are pinned, then not much to do. If not, do copy to user space function calls. } File ahash.c: static int crypto_ahash_us_setparam(struct crypto_tfm *tfm, void *param, u32 param_size) { struct crypto_ahash *atfm = __crypto_ahash_cast(tfm); struct uspace_ahash_param *usp = param; int rc; rc = crypto_ahash_setkey(atfm, usp->data, usp->hmackey_size); :::: return rc; } static int crypto_ahash_us_op(struct crypto_tfm *tfm, const struct iovec *iov, u32 iov_cnt, crypto_completion_t cb, void *ctx) { struct crypto_ahash * atfm = __crypto_ahash_cast(tfm); struct ahash_request * req; struct scatterlist * ssg; int len; int len_sg; int rc; len = sizeof(struct ahash_request) + crypto_ahash_reqsize(atfm); len = ALIGN(len, __alignof__(struct scatterlist)); len_sg = len; len += sizeof(struct scatterlist); req = kzalloc(len, GFP_KERNEL); if (req == NULL) return -ENOMEM; /* For this example, iov[0] is data to be hashed and iov[1] is result buffer */ ssg = (struct scatterlist *) ((u8 *) req + len_sg); sg_init_one(ssg, iov[0].iov_base, iov[0].iov_len); ahash_request_set_tfm(req, atfm); ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, cb, ctx); ahash_request_set_crypt(req, ssg, iov[1].iov_base, iov[0].iov_len); rc = crypto_ahash_digest(req); switch (rc) { case -EINPROGRESS: break; case 0: case -EBUSY: default: kfree(req); break; } return rc; } const struct crypto_type crypto_ahash_type = { .ctxsize = crypto_ahash_ctxsize, .init = crypto_init_ahash_ops, #ifdef CONFIG_PROC_FS .show = crypto_ahash_show, #endif .uspace_setparam = crypto_ahash_us_setparam, .uspace_op = crypto_ahash_us_op }; This addresses the algorithm abstraction as well as support for any CryptoAPI algorithm types. All user space buffers mapping to kernel space will still be handled by CryptoDev driver. This avoids creating a bunch of complication for each type of CryptoAPI type of algorithms. -Loc -----Original Message----- From: Herbert Xu [mailto:herbert@xxxxxxxxxxxxxxxxxxx] Sent: Tuesday, January 27, 2009 8:47 PM To: Shasi Pulijala Cc: Loc Ho Subject: Re: [PATCH 1/1 v8] Add CryptoAPI User Interface Support v8 On Tue, Jan 20, 2009 at 11:40:21PM -0800, Shasi Pulijala wrote: > Hi Herbert, > > Thanks for all your comments and suggestions. > As per your comments we have come up with the following design for the > user space design. The below are the structures and > API's that provide the user interface to the cryptodev implementation. > Could you please review this once and let me know > if it's a better abstract interface or if you see any problems with it. Thanks for the quick turn-around! > #define CRYPTODEV_NCRYPTO Max num of Operations If by operations you mean things like encrypt/decrypt/compress/decompress then we shouldn't have a global ID space for them. They should be per-type. That is, you have a separate ID space fir ciphers, hashes, and so on. > static struct cryptodev_link_handler > *cryptodev_msg_handlers[CRYPTODEV_NCRYPTO]; > int cryptodev_register_handlers(int protocol, > session_link_handler sessit, operation_link_handler > opit) Is this the kernel API? If so I don't think we need any new registration interface since we can just go through the existing crypto_type object. So what would happen is that upon initilisation user-space gives us a 4-tuple of (name, frontend type, type, mask). We use that to create the tfm object which would give us a pointer to the frontend type (note that we don't currently have that pointer but we should add it to crypto_tfm) that should then contain all the hooks to further parse the requests from user-space. After initilisation we would give user-space some sort of an ID to the tfm object so all subsequent operations simply need to give us the ID to get to the tfm and the rest of the info. In fact we can just generate one file descriptor per tfm since that gives us a natural limit on how many tfms you can allocate, plus you can poll on it without being overwhelmed. > struct cryptodev_cipher_op { > #define COP_NONE 0 > #define COP_ENCRYPT 1 > #define COP_DECRYPT 2 > __u16 op; /* i.e. COP_ENCRYPT */ > __u16 flags; > __u16 iv_size; > __u8 data[0]; > } It's probably best to separate the meta-data from the data since making them contiguous is not always easy. But otherwise this looks fine to me. > /** For Session **/ > > struct cryptodev_param_algo { > int type; > int param_len; > char param[0]; > }; > > struct cryptodev_cipher_algo { > char alg_name[64]; > __u16 alg_key_len; > char alg_key[0]; > }; > > struct cryptodev_aead_algo { > char alg_name[64]; > __u16 alg_key_len; > __u16 alg_icv_len; > char alg_key[0]; > }; As I said above, I think we should have a generic 4-tuple struct that is used to allocate the tfm, after which we can just use the file descriptor. > Session Related API's > > static int cryptodev_ioctl(struct inode *inode, struct file *filp, > unsigned int cmd, unsigned long arg) > { > / *This will called when the user does an ioctl session creation */ > This routine will do: > Creating the session, > copy_from_user(cryptodev_param_algo, (void *) arg, sizeof(struct > cryptodev_param_algo)) > Check the type value in cryptodev_param_algo structure, and call > the appropriate > session creation handler > > } Right, I think we really only need to one ioctl to allocate the tfm, which returns a new file descriptor and everything else can be done with read/write. Thanks, -- Visit Openswan at http://www.openswan.org/ Email: Herbert Xu ~{PmV>HI~} <herbert@xxxxxxxxxxxxxxxxxxx> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html