[dpdk-dev,1/8] crypto/aesni_gcm: do not append digest
Checks
Commit Message
When performing an authentication verification,
the PMD was using memory at the end of the input buffer,
to store temporarily the digest.
This operation requires the buffer to have enough
tailroom unnecessarily.
Instead, memory is allocated for each queue pair, to store
temporarily the digest generated by the driver, so it can
be compared with the one provided in the crypto operation,
without needing to touch the input buffer.
Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
---
drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 31 +++++-------------------
drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h | 7 ++++++
2 files changed, 13 insertions(+), 25 deletions(-)
Comments
Hi Pablo,
Thanks for the patch. It is very good idea of allocating only necessary buffer for digests in the operation.
Comments inline:
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> Sent: Friday, August 18, 2017 8:21 AM
> To: Doherty, Declan <declan.doherty@intel.com>;
> jerin.jacob@caviumnetworks.com
> Cc: dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Subject: [dpdk-dev] [PATCH 1/8] crypto/aesni_gcm: do not append digest
>
> When performing an authentication verification, the PMD was using memory
> at the end of the input buffer, to store temporarily the digest.
> This operation requires the buffer to have enough tailroom unnecessarily.
> Instead, memory is allocated for each queue pair, to store temporarily the
> digest generated by the driver, so it can be compared with the one provided
> in the crypto operation, without needing to touch the input buffer.
>
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> ---
> drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 31 +++++-------------------
> drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h | 7 ++++++
> 2 files changed, 13 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> index d9c91d0..ae670a7 100644
> --- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> +++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> @@ -298,14 +298,7 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp,
> struct rte_crypto_op *op,
> sym_op->aead.digest.data,
> (uint64_t)session->digest_length);
> } else if (session->op ==
> AESNI_GCM_OP_AUTHENTICATED_DECRYPTION) {
> - uint8_t *auth_tag = (uint8_t
> *)rte_pktmbuf_append(sym_op->m_dst ?
> - sym_op->m_dst : sym_op->m_src,
> - session->digest_length);
> -
> - if (!auth_tag) {
> - GCM_LOG_ERR("auth_tag");
> - return -1;
> - }
qp->tmp_digest has already the data type of uint8_t*, the casting is not necessary here. Although the "&" here seems to be wrong. auth_tag didn't point to qp->tmp_digest but a buffer with the address as &qp->tmp_digest.
> + uint8_t *auth_tag = (uint8_t *)&qp->temp_digest;
> qp->ops[session->key].init(&session->gdata_key,
> &qp->gdata_ctx,
> @@ -350,14 +343,7 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp,
> struct rte_crypto_op *op,
> sym_op->auth.digest.data,
> (uint64_t)session->digest_length);
> } else { /* AESNI_GMAC_OP_VERIFY */
> - uint8_t *auth_tag = (uint8_t
> *)rte_pktmbuf_append(sym_op->m_dst ?
> - sym_op->m_dst : sym_op->m_src,
> - session->digest_length);
> -
> - if (!auth_tag) {
> - GCM_LOG_ERR("auth_tag");
> - return -1;
> - }
Same here.
> + uint8_t *auth_tag = (uint8_t *)&qp->temp_digest;
>
> qp->ops[session->key].init(&session->gdata_key,
> &qp->gdata_ctx,
> @@ -385,11 +371,10 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp,
> struct rte_crypto_op *op,
> * - Returns NULL on invalid job
> */
> static void
> -post_process_gcm_crypto_op(struct rte_crypto_op *op,
> +post_process_gcm_crypto_op(struct aesni_gcm_qp *qp,
> + struct rte_crypto_op *op,
> struct aesni_gcm_session *session)
> {
> - struct rte_mbuf *m = op->sym->m_dst ? op->sym->m_dst : op-
> >sym->m_src;
> -
> op->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
>
> /* Verify digest if required */
> @@ -397,8 +382,7 @@ post_process_gcm_crypto_op(struct rte_crypto_op
> *op,
> session->op == AESNI_GMAC_OP_VERIFY) {
> uint8_t *digest;
>
Same here.
> - uint8_t *tag = rte_pktmbuf_mtod_offset(m, uint8_t *,
> - m->data_len - session->digest_length);
> + uint8_t *tag = (uint8_t *)&qp->temp_digest;
>
...
>
> --
> 2.9.4
The same problem lies the rest of your patches in the patchset.
Regards,
Fan
Hi Fan,
> -----Original Message-----
> From: Zhang, Roy Fan
> Sent: Monday, September 4, 2017 11:08 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Subject: RE: [dpdk-dev] [PATCH 1/8] crypto/aesni_gcm: do not append
> digest
>
> Hi Pablo,
>
> Thanks for the patch. It is very good idea of allocating only necessary buffer
> for digests in the operation.
> Comments inline:
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pablo de Lara
> > Sent: Friday, August 18, 2017 8:21 AM
> > To: Doherty, Declan <declan.doherty@intel.com>;
> > jerin.jacob@caviumnetworks.com
> > Cc: dev@dpdk.org; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Subject: [dpdk-dev] [PATCH 1/8] crypto/aesni_gcm: do not append digest
> >
> > When performing an authentication verification, the PMD was using
> > memory at the end of the input buffer, to store temporarily the digest.
> > This operation requires the buffer to have enough tailroom unnecessarily.
> > Instead, memory is allocated for each queue pair, to store temporarily
> > the digest generated by the driver, so it can be compared with the one
> > provided in the crypto operation, without needing to touch the input
> buffer.
> >
> > Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > ---
> > drivers/crypto/aesni_gcm/aesni_gcm_pmd.c | 31 +++++----------------
> ---
> > drivers/crypto/aesni_gcm/aesni_gcm_pmd_private.h | 7 ++++++
> > 2 files changed, 13 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> > b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> > index d9c91d0..ae670a7 100644
> > --- a/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> > +++ b/drivers/crypto/aesni_gcm/aesni_gcm_pmd.c
> > @@ -298,14 +298,7 @@ process_gcm_crypto_op(struct aesni_gcm_qp
> *qp,
> > struct rte_crypto_op *op,
> > sym_op->aead.digest.data,
> > (uint64_t)session->digest_length);
> > } else if (session->op ==
> > AESNI_GCM_OP_AUTHENTICATED_DECRYPTION) {
> > - uint8_t *auth_tag = (uint8_t
> > *)rte_pktmbuf_append(sym_op->m_dst ?
> > - sym_op->m_dst : sym_op->m_src,
> > - session->digest_length);
> > -
> > - if (!auth_tag) {
> > - GCM_LOG_ERR("auth_tag");
> > - return -1;
> > - }
>
> qp->tmp_digest has already the data type of uint8_t*, the casting is not
> necessary here. Although the "&" here seems to be wrong. auth_tag didn't
> point to qp->tmp_digest but a buffer with the address as &qp->tmp_digest.
Thanks for spotting this! Will send a v2 soon.
Pablo
@@ -298,14 +298,7 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp, struct rte_crypto_op *op,
sym_op->aead.digest.data,
(uint64_t)session->digest_length);
} else if (session->op == AESNI_GCM_OP_AUTHENTICATED_DECRYPTION) {
- uint8_t *auth_tag = (uint8_t *)rte_pktmbuf_append(sym_op->m_dst ?
- sym_op->m_dst : sym_op->m_src,
- session->digest_length);
-
- if (!auth_tag) {
- GCM_LOG_ERR("auth_tag");
- return -1;
- }
+ uint8_t *auth_tag = (uint8_t *)&qp->temp_digest;
qp->ops[session->key].init(&session->gdata_key,
&qp->gdata_ctx,
@@ -350,14 +343,7 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp, struct rte_crypto_op *op,
sym_op->auth.digest.data,
(uint64_t)session->digest_length);
} else { /* AESNI_GMAC_OP_VERIFY */
- uint8_t *auth_tag = (uint8_t *)rte_pktmbuf_append(sym_op->m_dst ?
- sym_op->m_dst : sym_op->m_src,
- session->digest_length);
-
- if (!auth_tag) {
- GCM_LOG_ERR("auth_tag");
- return -1;
- }
+ uint8_t *auth_tag = (uint8_t *)&qp->temp_digest;
qp->ops[session->key].init(&session->gdata_key,
&qp->gdata_ctx,
@@ -385,11 +371,10 @@ process_gcm_crypto_op(struct aesni_gcm_qp *qp, struct rte_crypto_op *op,
* - Returns NULL on invalid job
*/
static void
-post_process_gcm_crypto_op(struct rte_crypto_op *op,
+post_process_gcm_crypto_op(struct aesni_gcm_qp *qp,
+ struct rte_crypto_op *op,
struct aesni_gcm_session *session)
{
- struct rte_mbuf *m = op->sym->m_dst ? op->sym->m_dst : op->sym->m_src;
-
op->status = RTE_CRYPTO_OP_STATUS_SUCCESS;
/* Verify digest if required */
@@ -397,8 +382,7 @@ post_process_gcm_crypto_op(struct rte_crypto_op *op,
session->op == AESNI_GMAC_OP_VERIFY) {
uint8_t *digest;
- uint8_t *tag = rte_pktmbuf_mtod_offset(m, uint8_t *,
- m->data_len - session->digest_length);
+ uint8_t *tag = (uint8_t *)&qp->temp_digest;
if (session->op == AESNI_GMAC_OP_VERIFY)
digest = op->sym->auth.digest.data;
@@ -414,9 +398,6 @@ post_process_gcm_crypto_op(struct rte_crypto_op *op,
if (memcmp(tag, digest, session->digest_length) != 0)
op->status = RTE_CRYPTO_OP_STATUS_AUTH_FAILED;
-
- /* trim area used for digest from mbuf */
- rte_pktmbuf_trim(m, session->digest_length);
}
}
@@ -435,7 +416,7 @@ handle_completed_gcm_crypto_op(struct aesni_gcm_qp *qp,
struct rte_crypto_op *op,
struct aesni_gcm_session *sess)
{
- post_process_gcm_crypto_op(op, sess);
+ post_process_gcm_crypto_op(qp, op, sess);
/* Free session if a session-less crypto op */
if (op->sess_type == RTE_CRYPTO_OP_SESSIONLESS) {
@@ -58,6 +58,8 @@
#define GCM_LOG_DBG(fmt, args...)
#endif
+/* Maximum length for digest */
+#define DIGEST_LENGTH_MAX 16
/** private data structure for each virtual AESNI GCM device */
struct aesni_gcm_private {
@@ -84,6 +86,11 @@ struct aesni_gcm_qp {
/**< Queue Pair Identifier */
char name[RTE_CRYPTODEV_NAME_LEN];
/**< Unique Queue Pair Name */
+ uint8_t temp_digest[DIGEST_LENGTH_MAX];
+ /**< Buffer used to store the digest generated
+ * by the driver when verifying a digest provided
+ * by the user (using authentication verify operation)
+ */
} __rte_cache_aligned;