From: "Stephan Müller" <smueller@chronox.de>
To: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Cc: herbert@gondor.apana.org.au, davem@davemloft.net,
nicolas.ferre@atmel.com, linux-crypto@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 11/12] crypto: atmel-authenc: add support to authenc(hmac(shaX),Y(aes)) modes
Date: Mon, 09 Jan 2017 19:34:36 +0100 [thread overview]
Message-ID: <1593848.VVZBeNLOu9@tauon.atsec.com> (raw)
In-Reply-To: <6301d79c-f1c5-d86c-823c-dfdbb5100e74@atmel.com>
Am Montag, 9. Januar 2017, 19:24:12 CET schrieb Cyrille Pitchen:
Hi Cyrille,
> >> +static int atmel_aes_authenc_copy_assoc(struct aead_request *req)
> >> +{
> >> + size_t buflen, assoclen = req->assoclen;
> >> + off_t skip = 0;
> >> + u8 buf[256];
> >> +
> >> + while (assoclen) {
> >> + buflen = min_t(size_t, assoclen, sizeof(buf));
> >> +
> >> + if (sg_pcopy_to_buffer(req->src, sg_nents(req->src),
> >> + buf, buflen, skip) != buflen)
> >> + return -EINVAL;
> >> +
> >> + if (sg_pcopy_from_buffer(req->dst, sg_nents(req->dst),
> >> + buf, buflen, skip) != buflen)
> >> + return -EINVAL;
> >> +
> >> + skip += buflen;
> >> + assoclen -= buflen;
> >> + }
> >
> > This seems to be a very expansive operation. Wouldn't it be easier, leaner
> > and with one less memcpy to use the approach of
> > crypto_authenc_copy_assoc?
> >
> > Instead of copying crypto_authenc_copy_assoc, what about carving the logic
> > in crypto/authenc.c out into a generic aead helper code as we need to add
> > that to other AEAD implementations?
>
> Before writing this function, I checked how the crypto/authenc.c driver
> handles the copy of the associated data, hence crypto_authenc_copy_assoc().
>
> I have to admit I didn't perform any benchmark to compare the two
> implementation but I just tried to understand how
> crypto_authenc_copy_assoc() works. At the first look, this function seems
> very simple but I guess all the black magic is hidden by the call of
> crypto_skcipher_encrypt() on the default null transform, which is
> implemented using the ecb(cipher_null) algorithm.
The magic in the null cipher is that it not only performs a memcpy, but
iterates through the SGL and performs a memcpy on each part of the source/
destination SGL.
I will release a patch set later today -- the coding is completed, but testing
is yet under way. That patch now allows you to make only one function call
without special init/deinit code.
>
> When I wrote my function I thought that this ecb(cipher_null) algorithm was
> implemented by combining crypto_ecb_crypt() from crypto/ecb.c with
> null_crypt() from crypto/crypto_null.c. Hence I thought there would be much
> function call overhead to copy only few bytes but now checking again I
> realize that the ecb(cipher_null) algorithm is directly implemented by
> skcipher_null_crypt() still from crypto/crypto_null.c. So yes, maybe you're
> right: it could be better to reuse what was done in
> crypto_authenc_copy_assoc() from crypto/authenc.c.
>
> This way we could need twice less memcpy() hence I agree with you.
In addition to the additional memcpy, the patch I want to air shortly (and
which I hope is going to be accepted) should reduce the complexity of your
code in this corner.
...
> >> +static int atmel_aes_authenc_crypt(struct aead_request *req,
> >> + unsigned long mode)
> >> +{
> >> + struct atmel_aes_authenc_reqctx *rctx = aead_request_ctx(req);
> >> + struct crypto_aead *tfm = crypto_aead_reqtfm(req);
> >> + struct atmel_aes_base_ctx *ctx = crypto_aead_ctx(tfm);
> >> + u32 authsize = crypto_aead_authsize(tfm);
> >> + bool enc = (mode & AES_FLAGS_ENCRYPT);
> >> + struct atmel_aes_dev *dd;
> >> +
> >> + /* Compute text length. */
> >> + rctx->textlen = req->cryptlen - (enc ? 0 : authsize);
> >
> > Is there somewhere a check that authsize is always < req->cryptlen (at
> > least it escaped me)? Note, this logic will be exposed to user space
> > which may do funky things.
>
> I thought those 2 sizes were always set by the kernel only but I admit I
> didn't check my assumption. If you tell me they could be set directly from
> the userspace, yes I agree with you, I need to add a test.
Then I would like to ask you adding that check -- as this check is cheap, it
should not affect performance.
Ciao
Stephan
next prev parent reply other threads:[~2017-01-09 18:34 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-22 16:37 [PATCH v2 00/12] crypto: atmel-authenc: add support to authenc(hmac(shaX),Y(aes)) modes Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 01/12] crypto: atmel-sha: create function to get an Atmel SHA device Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 02/12] crypto: atmel-sha: update request queue management to make it more generic Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 03/12] crypto: atmel-sha: make atmel_sha_done_task " Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 04/12] crypto: atmel-sha: redefine SHA_FLAGS_SHA* flags to match SHA_MR_ALGO_SHA* Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 05/12] crypto: atmel-sha: add atmel_sha_wait_for_data_ready() Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 06/12] crypto: atmel-sha: add SHA_MR_MODE_IDATAR0 Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 07/12] crypto: atmel-sha: add atmel_sha_cpu_start() Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 08/12] crypto: atmel-sha: add simple DMA transfers Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 09/12] crypto: atmel-sha: add support to hmac(shaX) Cyrille Pitchen
2016-12-22 16:37 ` [PATCH v2 10/12] crypto: atmel-aes: fix atmel_aes_handle_queue() Cyrille Pitchen
2016-12-22 16:38 ` [PATCH v2 11/12] crypto: atmel-authenc: add support to authenc(hmac(shaX),Y(aes)) modes Cyrille Pitchen
2016-12-22 21:10 ` kbuild test robot
2016-12-23 11:34 ` [PATCH v2 11/12] crypto: atmel-authenc: add support to authenc(hmac(shaX), Y(aes)) modes Stephan Müller
2017-01-09 18:24 ` [PATCH v2 11/12] crypto: atmel-authenc: add support to authenc(hmac(shaX),Y(aes)) modes Cyrille Pitchen
2017-01-09 18:34 ` Stephan Müller [this message]
2016-12-22 16:38 ` [PATCH v2 12/12] crypto: atmel-sha: add verbose debug facilities to print hw register names Cyrille Pitchen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1593848.VVZBeNLOu9@tauon.atsec.com \
--to=smueller@chronox.de \
--cc=cyrille.pitchen@atmel.com \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolas.ferre@atmel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).