LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephan Mueller <smueller@chronox.de>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "'Quentin Gouchet'" <quentin.gouchet@gmail.com>,
	Daniel Borkmann <dborkman@redhat.com>,
	linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	linux-crypto@vger.kernel.org
Subject: Re: [PATCH v8 1/2] crypto: AF_ALG: add AEAD support
Date: Fri, 09 Jan 2015 04:30:45 +0100	[thread overview]
Message-ID: <1639027.yRSjDuRfFC@tachyon.chronox.de> (raw)
In-Reply-To: <20150108110931.GA8568@gondor.apana.org.au>

Am Donnerstag, 8. Januar 2015, 22:09:31 schrieb Herbert Xu:

Hi Herbert,

> On Wed, Jan 07, 2015 at 04:51:38PM +0100, Stephan Mueller wrote:
> > +		if (!aead_writable(sk)) {
> > +			/*
> > +			 * If there is more data to be expected, but we cannot
> > +			 * write more data, forcefully define that we do not
> > +			 * expect more data to invoke the AEAD operation. This
> > +			 * prevents a deadlock in user space.
> > +			 */
> > +			ctx->more = 0;
> 
> We should return EMSGSIZE here.  Also we should clear out the
> existing data so that the socket may be reused again.

Is this really wise considering that we want to support a threaded caller? For 
example, one thread sends data and another reads data. For some reason, the 
reading thread is throttled or slower than the sender. Now, with the current 
solution, the sender is put on hold (i.e. throttled) until the reader can 
catch up. I.e. we have an automated synchronization between sender/receiver.

Thus, when we remove the wait here and return an error, the sender will be 
shut down and there is no synchronization of the reader/writer any more.

Note, the same applies to the very similar code in aead_sendpage too.
> 
> > +	ctx->more = msg->msg_flags & MSG_MORE;
> > +	if (!ctx->more && !aead_sufficient_data(ctx))
> > +		err = -EINVAL;
> 
> Ditto, we should discard the data that's queued up.  Also perhaps
> use EBADMSG instead of EINVAL.

Agreed that we should clear out the buffer. I will provide that in the next 
release for both, the sendmsg and sendpage implementations.

However, I am not sure whether using EBADMSG is a good idea. The error of 
EBADMSG in the kernel crypto API is only used for integrity errors of AEAD 
ciphers. But our error case here has nothing to do with the integrity error.

I would be fine with any other error number -- EMSGSIZE as you suggested 
above?
> 
> > +	/*
> > +	 * Require exactly one IOV block as the AEAD operation is a one shot
> > +	 * due to the authentication tag.
> > +	 */
> > +	if (msg->msg_iter.nr_segs != 1)
> > +		return -ENOMSG;
> 
> Why does the receive buffer have to be contiguous?

I thought for quite some time about how we can use multiple iovecs. But I 
found no satisfactory solution. The solution I see is described below.

If we consider the skcipher implementation, the code iterates over the iovecs 
as the outermost loop. In each loop iteration the cipher operation is 
triggered.

Now in case of AEAD, all provided data the kernel received has to be operated 
on with exactly one cipher invocation. Looking at skcipher, that would mean 
that we only perform one loop iteration, i.e. processing exactly one iovec.

A possible solution would be that I use an array of struct af_alg_sgl and 
iterate over that array for each iovec. Something like the following:

struct aead_ctx {
...
	struct af_alg_sgl rsgl[ALG_MAX_PAGES];

for(i = 0; i < ALG_MAX_PAGES; i++) {
	af_alg_make_sg(rsgl[i], iov[i])
	scatterwalk_sg_chain(rsgl[i-1].sg rsgl[i]);
}

But my concern is that with the array of rsgl we occupy a sizable amount of 
memory as af_alg_sgl again defines arrays of entries.

Do you think whether such approach makes sense? If yes, which limit to the 
number of rsgl should we apply -- is ALG_MAX_PAGES good?

-- 
Ciao
Stephan

  reply	other threads:[~2015-01-09  3:30 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-07 15:51 [PATCH v8 0/2] crypto: AF_ALG: add AEAD and RNG support Stephan Mueller
2015-01-07 15:51 ` [PATCH v8 1/2] crypto: AF_ALG: add AEAD support Stephan Mueller
2015-01-08 11:09   ` Herbert Xu
2015-01-09  3:30     ` Stephan Mueller [this message]
2015-01-20  3:00       ` Herbert Xu
2015-01-20  3:08         ` Stephan Mueller
2015-01-07 15:52 ` [PATCH v8 2/2] crypto: AF_ALG: enable AEAD interface compilation Stephan Mueller

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=1639027.yRSjDuRfFC@tachyon.chronox.de \
    --to=smueller@chronox.de \
    --cc=dborkman@redhat.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=quentin.gouchet@gmail.com \
    --subject='Re: [PATCH v8 1/2] crypto: AF_ALG: add AEAD support' \
    /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

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).