Re: [RFC PATCH] crypto: pcrypt - forbid recursive instantiation

[Steffen Klassert]

On Mon, Apr 09, 2018 at 12:07:39PM -0700, Eric Biggers wrote:
> On Mon, Apr 09, 2018 at 10:58:08AM +0200, Steffen Klassert wrote:
> > On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote:
> > > 
> > > Steffen, how feasible do you think would it be to make each pcrypt 
> > > instance use
> > > its own padata instance, so different pcrypt instances aren't ordered with
> > > respect to each other?
> > 
> > It should be possible at least, but requires some work.
> > I already had patches to get multiple independent pcrypt
> > insatance to better support pcrypt for different workloads
> > and NUMA mchines. I never got finalized with is because
> > of the lack of NUMA hardware to test but the code is 
> > still availabe, maybe it can help:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt
> > 
> > > Or do you think there is a better solution?
> > 
> > Not really.
> > 
> > Btw. is there a valid usecase where a certain template
> > is used twice in one algorithm instance?
> > 
> 
> None that I can think of right now.  The problem is that it's hard to prevent
> template recursion when users can specify arbitrary algorithms using AF_ALG, 
> and
> some algorithms even use fallbacks which may use templates not explicitly 
> listed
> in the driver name.
> 
> Remember, a kernel deadlock is a bug even if it's "not a valid use case"...
> In this case it can even be triggered by an unprivileged user via AF_ALG.

Yes sure, I just wanted to know if it is worth to think about
preventing template recursions. If there is a valid usecase,
then we don't even need to think in this direction.

While I think each pcrypt instance should have it's own
padata instance on the long run, it would be good to have
a not so intrusive fix that can be backported to the stable
trees.

Re: [RFC PATCH] crypto: pcrypt - forbid recursive instantiation

[Steffen Klassert]

On Sun, Apr 08, 2018 at 03:55:28PM -0700, Eric Biggers wrote:
> On Fri, Mar 23, 2018 at 08:21:52AM +0800, Herbert Xu wrote:
> > On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote:
> > > From: Eric Biggers 
> > > 
> > > If the pcrypt template is used multiple times in an algorithm, then a
> > > deadlock occurs because all pcrypt instances share the same
> > > padata_instance, which completes requests in the order submitted.  That
> > > is, the inner pcrypt request waits for the outer pcrypt request while
> > > the outer request is already waiting for the inner.
> > > 
> > > Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> > > underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
> > > simple fix that should be sufficient to prevent the deadlock.
> > 
> > I'm a bit uneasy with this fix.  What if pcrypt is used in the
> > underlying algorithm as a fallback? Wouldn't it still dead-lock
> > without triggering this check?
> > 
> 
> Yep, I think you're right.
> 
> Steffen, how feasible do you think would it be to make each pcrypt instance 
> use
> its own padata instance, so different pcrypt instances aren't ordered with
> respect to each other?

It should be possible at least, but requires some work.
I already had patches to get multiple independent pcrypt
insatance to better support pcrypt for different workloads
and NUMA mchines. I never got finalized with is because
of the lack of NUMA hardware to test but the code is 
still availabe, maybe it can help:

https://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-pcrypt

> Or do you think there is a better solution?

Not really.

Btw. is there a valid usecase where a certain template
is used twice in one algorithm instance?

Re: [RFC PATCH] crypto: pcrypt - forbid recursive instantiation

[Steffen Klassert]

On Sat, Mar 10, 2018 at 03:22:31PM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebigg...@google.com>
> 
> If the pcrypt template is used multiple times in an algorithm, then a
> deadlock occurs because all pcrypt instances share the same
> padata_instance, which completes requests in the order submitted.  That
> is, the inner pcrypt request waits for the outer pcrypt request while
> the outer request is already waiting for the inner.
> 
> Fix this by making pcrypt forbid instantiation if pcrypt appears in the
> underlying ->cra_driver_name.  This is somewhat of a hack, but it's a
> simple fix that should be sufficient to prevent the deadlock.
> 
> Reproducer:
> 
>   #include 
>   #include 
>   #include 
> 
>   int main()
>   {
>   struct sockaddr_alg addr = {
>   .salg_type = "aead",
>   .salg_name = "pcrypt(pcrypt(rfc4106-gcm-aesni))"
>   };
>   int algfd, reqfd;
>   char buf[32] = { 0 };
> 
>   algfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
>   bind(algfd, (void *), sizeof(addr));
>   setsockopt(algfd, SOL_ALG, ALG_SET_KEY, buf, 20);
>   reqfd = accept(algfd, 0, 0);
>   write(reqfd, buf, 32);
>   read(reqfd, buf, 16);
>   }
> 
> Reported-by: 
> syzbot+56c7151cad94eec37c521f0e47d2eee53f936...@syzkaller.appspotmail.com
> Fixes: 5068c7a883d1 ("crypto: pcrypt - Add pcrypt crypto parallelization 
> wrapper")
> Cc: <sta...@vger.kernel.org> # v2.6.34+
> Signed-off-by: Eric Biggers <ebigg...@google.com>
> ---
>  crypto/pcrypt.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
> index f8ec3d4ba4a80..3ec64604f6a56 100644
> --- a/crypto/pcrypt.c
> +++ b/crypto/pcrypt.c
> @@ -265,6 +265,12 @@ static void pcrypt_free(struct aead_instance *inst)
>  static int pcrypt_init_instance(struct crypto_instance *inst,
>   struct crypto_alg *alg)
>  {
> + /* Recursive pcrypt deadlocks due to the shared padata_instance */
> + if (!strncmp(alg->cra_driver_name, "pcrypt(", 7) ||
> + strstr(alg->cra_driver_name, "(pcrypt(") ||
> + strstr(alg->cra_driver_name, ",pcrypt("))
> + return -EINVAL;
> +

This looks ok to me.

Acked-by: Steffen Klassert <steffen.klass...@secunet.com>

Re: [PATCH v2 0/4] crypto: aesni - Use zero-copy for gcm(aes) buffers that are partially contiguous

[Steffen Klassert]

On Wed, Jan 31, 2018 at 10:54:57AM -0800, Junaid Shahid wrote:
> On Wed, Jan 31, 2018 at 12:13 AM, Steffen Klassert
> <steffen.klass...@secunet.com> wrote:
> >
> > I wonder which special usecase you have in mind that will be improved
> > by your patches.
> >
> 
> This is not related to IPsec. We have an internal use case where the
> data buffer itself is a single memory page but the authentication tag
> needs to be in a separate buffer. This patch set allows us to have
> zero copy in that case.

We usually don't add code for 'internal use cases' to the mainline
kernel. If you need a SG version of gcm-aesni, then please implement
the generic version so that everybody can benefit from it.

Think about how this will look like if everybody would add
a workaround for its very special use case.

Re: [PATCH 1/2] crypto: Implement a generic crypto statistics

[Steffen Klassert]

On Fri, Jan 26, 2018 at 11:43:28PM +0800, Herbert Xu wrote:
> On Thu, Jan 18, 2018 at 09:58:13AM +0100, LABBE Corentin wrote:
> >
> > I have two way of adding a new netlink request
> > - keep the current patch and simply add a new CRYPTO_MSG_GETSTAT which use 
> > the same function than CRYPTO_MSG_GETALG
> > => minimal changes, in fact CRYPTO_MSG_GETSTAT and CRYPTO_MSG_GETALG 
> > would be the same, but it is easy for userspace to test presence of stat.
> > - Create a new CRYPTO_MSG_GETSTAT which imply lot of code and add a new 
> > crypto_user_stat.c
> > => this imply also to change makefile (rename crypto_user.c to 
> > crypto_user_base.c) since crypto_user.ko is made of two files.
> > 
> > Which one do you prefer ?
> 
> I think you should check with the crconf author, Steffen Klassert
> since that will be affected by this change.

I still haven't had time to look into the details, but I'm
fine with everything that does not break existing userspace.
Everything else would be a 'no go'.

Re: [PATCH v2 0/4] crypto: aesni - Use zero-copy for gcm(aes) buffers that are partially contiguous

[Steffen Klassert]

Hi Junaid.

On Tue, Jan 23, 2018 at 12:19:12PM -0800, Junaid Shahid wrote:
> Changes in v2:
> - Integrated https://patchwork.kernel.org/patch/10173981
> 
> Currently, the AESNI gcm(aes) implementation uses zero-copy only when the
> entire src and dest request buffers, including the AAD, the data and the
> Auth Tag are contiguous. This series enables the use of zero-copy even if the
> AAD and/or Auth Tag are in different buffers than the actual data, as long as
> each of them individually satisfies the zero-copy conditions (i.e. the entire
> buffer is either in low-mem or within a single high-mem page). Furthermore,
> it also enables the use of zero-copy even if only one of src and dest 
> satisfies
> these conditions rather than only when both of them do.

I wonder which special usecase you have in mind that will be improved
by your patches.

For IPsec, the crypto layer either gets a contiguous buffer because
networking already linearized the packet data, or the packet data
and IPsec trailer are scattered over multiple page fragments.
So in the first case, it is OK already without your patches. In
the second case, your zero-copy conditions are not satisfied.

This looks a bit like a workaround of the real problem, that is
that the gcm-aesni implementation does not support SG operations.

Maybe it would be better to investigate in the direction to
support SG operations with gcm-aesni instead of trying to
find all corner cases where the existing implementation can
do zero-copy.

Re: INFO: task hung in aead_recvmsg

[Steffen Klassert]

On Sat, Dec 23, 2017 at 02:29:42PM -0600, Eric Biggers wrote:
> [+Cc Steffen Klassert <steffen.klass...@secunet.com>]
> 
> 
> I was able to reproduce this by trying to use 'pcrypt' recursively.  I am not
> 100% sure it is the exact same bug, but it probably is.  Here is a C 
> reproducer:
> 
>   #include 
>   #include 
>   #include 
> 
>   int main()
>   {
>   struct sockaddr_alg addr = {
>   .salg_type = "aead",
>   .salg_name = "pcrypt(pcrypt(rfc4106-gcm-aesni))",
>   };
>   int algfd, reqfd;
>   char buf[32] = { 0 };
> 
>   algfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
>   bind(algfd, (void *), sizeof(addr));
>   setsockopt(algfd, SOL_ALG, ALG_SET_KEY, buf, 20);
> 
>   reqfd = accept(algfd, 0, 0);
>   write(reqfd, buf, 32);
>   read(reqfd, buf, 16);
>   }
> 
> It seems the problem is that all 'pcrypt' instances use the same
> 'padata_instance', which completes works in the order they are submitted.  But
> with nested use, the outer work is submitted before the inner work, so the 
> inner
> work isn't allowed to complete until the outer work does, which deadlocks
> because actually the inner work needs to complete first.
> 
> What a mess.  Maybe there should be a separate 'padata_instance' per pcrypt
> instance?  Or maybe there should be a way for an algorithm to declare that it
> can only appear in the stack one time?  

Having two nested pcrypt templates in one algorithm instance
does not make so much sense in the first place. I thought
that the crypto layer would refuse to build an instance
with two nested templates of the same type.

At least for pcrypt, refusing such instantiations would
be the correct behaviour. Are there any other templates
where a nested use would make sense?

Re: [PATCH] padata: add SPDX identifier

[Steffen Klassert]

On Thu, Dec 28, 2017 at 05:53:27PM +1100, Herbert Xu wrote:
> Cheah Kok Cheong <thrus...@gmail.com> wrote:
> > Add SPDX license identifier according to the type of license text found
> > in the file.
> > 
> > Cc: Philippe Ombredanne <pombreda...@nexb.com>
> > Signed-off-by: Cheah Kok Cheong <thrus...@gmail.com>
> > ---
> > kernel/padata.c | 1 +
> > 1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index e265953..eb9a9d9 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > /*
> >  * padata.c - generic interface to process data streams in parallel
> >  *
> 
> Steffen, are you OK with this patch?

Yes, I'm ok with this.

Acked-by: Steffen Klassert <steffen.klass...@secunet.com>

Re: [PATCH 0/3] padata cpu awareness fixes

[Steffen Klassert]

On Fri, Sep 08, 2017 at 08:57:08PM +0200, Mathias Krause wrote:
> Hi Steffen, Herbert,
> 
> this series solves multiple issues of padata I ran into while trying to
> make use of pcrypt for IPsec.
> 
> The first issue is that the reorder timer callback may run on a CPU that
> isn't part of the parallel set, as it runs on the CPU where the timer
> interrupt gets handled. As a result, padata_reorder() may run on a CPU
> with uninitialized per-cpu parallel queues, so the __this_cpu_read() in
> padata_get_next() refers to an uninitialized cpu_index. However, as
> per-cpu memory gets memset(0) on allocation time, it'll read 0. If the
> next CPU index we're waiting for is 0 too, the compare will succeed and
> we'll return with -ENODATA, making padata_reorder() think there's
> nothing to do, stop any pending timer and return immediately. This is
> wrong as the pending timer might have been the one to trigger the needed
> reordering, but, as it was canceled, will never happen -- if no other
> parallel requests arrive afterwards.
> 
> That issue is addressed with the first two patches, ensuring we're not
> using a bogus cpu_index value for the compare and thereby not wrongly
> cancel a pending timer. The second patch then ensures the reorder timer
> callback runs on the correct CPU or, at least, on a CPU from the
> parallel set to generate forward progress.
> 
> The third patch addresses a similar issues for the serialization
> callback. We implicitly require padata_do_serial() to be called on the
> very same CPU padata_do_parallel() was called on to ensure the correct
> ordering of requests -- and correct functioning of padata, even. If the
> algorithm we're parallelizing is asynchronous itself, that invariant
> might not be true, as, e.g. crypto hardware may execute the callback on
> the CPU its interrupt gets handled on which isn't necessarily the one
> the request got issued on.
> 
> To handle that issue we track the CPU we're supposed to handle the
> request on and ensure we'll call padata_reorder() on that CPU when
> padata_do_serial() gets called -- either by already running on the
> corect CPU or by deferring the call to a worker. 
> 
> Please apply!
> 
> Mathias Krause (3):
>   padata: set cpu_index of unused CPUs to -1
>   padata: ensure the reorder timer callback runs on the correct CPU
>   padata: ensure padata_do_serial() runs on the correct CPU

Looks good, thanks!

Acked-by: Steffen Klassert <steffen.klass...@secunet.com>

Re: [PATCH] Crypto_user: Make crypto user API available for all net ns

[Steffen Klassert]

On Thu, Jul 13, 2017 at 04:51:10PM +0200, Stephan Müller wrote:
> Am Donnerstag, 13. Juli 2017, 16:22:32 CEST schrieb Christian Langrock:
> 
> Hi Christian,
> 
> > With this patch it's possible to use crypto user API form all
> > network namespaces, not only form the initial net ns.
> 
> Is this wise?
> 
> The crypto_user interface allows root users to change settings in the kernel 
> with a global scope. For example, you can deregister ciphers, change the prio 
> of ciphers and so on. All of that is visible on a global scale and thus 
> should 
> not be possible from namespaces, IMHO.

It is possible to use crypto from all namespaces, so would be nice if
it would be possible to choose which algorithm to use. The problem is that
you can change the global crypto configuration from within a namespace
with this. Maybe crypto_alg_list etc. should be namespace aware first,
then each namespace can have its own configuration.

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Steffen Klassert]

On Tue, Jul 11, 2017 at 11:53:11AM -0700, Dave Watson wrote:
> On 07/11/17 08:29 AM, Steffen Klassert wrote:
> > Sorry for replying to old mail...
> > > +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> > > +{
> > 
> > ...
> > 
> > > +
> > > + if (!sw_ctx->aead_send) {
> > > + sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> > > + if (IS_ERR(sw_ctx->aead_send)) {
> > > + rc = PTR_ERR(sw_ctx->aead_send);
> > > + sw_ctx->aead_send = NULL;
> > > + goto free_rec_seq;
> > > + }
> > > + }
> > > +
> > 
> > When I look on how you allocate the aead transformation, it seems
> > that you should either register an asynchronous callback with
> > aead_request_set_callback(), or request for a synchronous algorithm.
> > 
> > Otherwise you will crash on an asynchronous crypto return, no?
> 
> The intention is for it to be synchronous, and gather directly from
> userspace buffers.  It looks like calling
> crypto_alloc_aead("gcm(aes)", 0, CRYPTO_ALG_ASYNC) is the correct way
> to request synchronous algorithms only?

Yes, but then you loose the aes-ni based algorithms because they are
asynchronous. If you want to have good crypto performance, it is
better to implement the asynchronous callbacks.

> 
> > Also, it seems that you have your scatterlists on a per crypto
> > transformation base istead of per crypto request. Is this intentional?
> 
> We hold the socket lock and only one crypto op can happen at a time,
> so we reuse the scatterlists.

This is OK as long as the crypto happens synchronous. But as said above,
I think this is not what you want.

Re: [PATCH v3 net-next 3/4] tls: kernel TLS support

[Steffen Klassert]

Sorry for replying to old mail...

On Wed, Jun 14, 2017 at 11:37:39AM -0700, Dave Watson wrote:
> +static int tls_do_encryption(struct tls_context *tls_ctx,
> +  struct tls_sw_context *ctx, size_t data_len,
> +  gfp_t flags)
> +{
> + unsigned int req_size = sizeof(struct aead_request) +
> + crypto_aead_reqsize(ctx->aead_send);
> + struct aead_request *aead_req;
> + int rc;
> +
> + aead_req = kmalloc(req_size, flags);
> + if (!aead_req)
> + return -ENOMEM;
> +
> + ctx->sg_encrypted_data[0].offset += tls_ctx->prepend_size;
> + ctx->sg_encrypted_data[0].length -= tls_ctx->prepend_size;
> +
> + aead_request_set_tfm(aead_req, ctx->aead_send);
> + aead_request_set_ad(aead_req, TLS_AAD_SPACE_SIZE);
> + aead_request_set_crypt(aead_req, ctx->sg_aead_in, ctx->sg_aead_out,
> +data_len, tls_ctx->iv);
> + rc = crypto_aead_encrypt(aead_req);
> +
> + ctx->sg_encrypted_data[0].offset -= tls_ctx->prepend_size;
> + ctx->sg_encrypted_data[0].length += tls_ctx->prepend_size;
> +
> + kfree(aead_req);
> + return rc;
> +}

...

> +int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx)
> +{

...

> +
> + if (!sw_ctx->aead_send) {
> + sw_ctx->aead_send = crypto_alloc_aead("gcm(aes)", 0, 0);
> + if (IS_ERR(sw_ctx->aead_send)) {
> + rc = PTR_ERR(sw_ctx->aead_send);
> + sw_ctx->aead_send = NULL;
> + goto free_rec_seq;
> + }
> + }
> +

When I look on how you allocate the aead transformation, it seems
that you should either register an asynchronous callback with
aead_request_set_callback(), or request for a synchronous algorithm.

Otherwise you will crash on an asynchronous crypto return, no?

Also, it seems that you have your scatterlists on a per crypto
transformation base istead of per crypto request. Is this intentional?

Re: [PATCH] padata: allow caller to control queue length

[Steffen Klassert]

On Thu, Apr 13, 2017 at 11:52:13AM +0200, Jason A. Donenfeld wrote:
> Allow users of padata to determine the queue length themselves, via this
> added helper function, so that we can later remove the hard-coded 1000-
> job limit. We thus add a helper function, and then move the limiting
> functionality to pcrypt-proper, since it was the only current consumer
> relying on the 1000-job limit. We do, however, impose a limit on padata
> so that the reference count does not have an integer overflow.
> 
> Signed-off-by: Jason A. Donenfeld 

Why do we need this? As long as we don't have a user that needs a
different limit, this patch adds just some useless code.

Re: [PATCH] padata: get_next is never NULL

[Steffen Klassert]

On Wed, Apr 12, 2017 at 10:40:19AM +0200, Jason A. Donenfeld wrote:
> Per Dan's static checker warning, the code that returns NULL was removed
> in 2010, so this patch updates the comments and fixes the code
> assumptions.
> 
> Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
> Reported-by: Dan Carpenter <dan.carpen...@oracle.com>

This looks ok,

Acked-by: Steffen Klassert <steffen.klass...@secunet.com>

Re: [PATCH 3/4] xfrm: Prepare for CRYPTO_MAX_ALG_NAME expansion

[Steffen Klassert]

On Thu, Apr 06, 2017 at 04:16:10PM +0800, Herbert Xu wrote:
> This patch fixes the xfrm_user code to use the actual array size
> rather than the hard-coded CRYPTO_MAX_ALG_NAME length.  This is
> because the array size is fixed at 64 bytes while we want to increase
> the in-kernel CRYPTO_MAX_ALG_NAME value.
> 
> Signed-off-by: Herbert Xu <herb...@gondor.apana.org.au>

Acked-by: Steffen Klassert <steffen.klass...@secunet.com>

Re: [PATCH 0/4] crypto: CRYPTO_MAX_ALG_NAME is too low

[Steffen Klassert]

On Thu, Apr 06, 2017 at 01:58:32PM -0700, David Miller wrote:
> From: Herbert Xu 
> Date: Thu, 6 Apr 2017 16:15:09 +0800
> 
> > As the final patch depends on all three it would be easiest if
> > we pushed the xfrm patch through the crypto tree.  Steffen/David?
> 
> No objections from me for this going through the crypto tree.

I have no objections too.

Re: [PATCH] padata: avoid race in reordering

[Steffen Klassert]

On Thu, Mar 23, 2017 at 12:24:43PM +0100, Jason A. Donenfeld wrote:
> Under extremely heavy uses of padata, crashes occur, and with list
> debugging turned on, this happens instead:
> 
> [87487.298728] WARNING: CPU: 1 PID: 882 at lib/list_debug.c:33
> __list_add+0xae/0x130
> [87487.301868] list_add corruption. prev->next should be next
> (b17abfc043d0), but was 8dba70872c80. (prev=8dba70872b00).
> [87487.339011]  [] dump_stack+0x68/0xa3
> [87487.342198]  [] ? console_unlock+0x281/0x6d0
> [87487.345364]  [] __warn+0xff/0x140
> [87487.348513]  [] warn_slowpath_fmt+0x4a/0x50
> [87487.351659]  [] __list_add+0xae/0x130
> [87487.354772]  [] ? _raw_spin_lock+0x64/0x70
> [87487.357915]  [] padata_reorder+0x1e6/0x420
> [87487.361084]  [] padata_do_serial+0xa5/0x120
> 
> padata_reorder calls list_add_tail with the list to which its adding
> locked, which seems correct:
> 
> spin_lock(>serial.lock);
> list_add_tail(>list, >serial.list);
> spin_unlock(>serial.lock);
> 
> This therefore leaves only place where such inconsistency could occur:
> if padata->list is added at the same time on two different threads.
> This pdata pointer comes from the function call to
> padata_get_next(pd), which has in it the following block:
> 
> next_queue = per_cpu_ptr(pd->pqueue, cpu);
> padata = NULL;
> reorder = _queue->reorder;
> if (!list_empty(>list)) {
>padata = list_entry(reorder->list.next,
>struct padata_priv, list);
>spin_lock(>lock);
>list_del_init(>list);
>atomic_dec(>reorder_objects);
>spin_unlock(>lock);
> 
>pd->processed++;
> 
>goto out;
> }
> out:
> return padata;
> 
> I strongly suspect that the problem here is that two threads can race
> on reorder list. Even though the deletion is locked, call to
> list_entry is not locked, which means it's feasible that two threads
> pick up the same padata object and subsequently call list_add_tail on
> them at the same time. The fix is thus be hoist that lock outside of
> that block.
> 
> Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>

Acked-by: Steffen Klassert <steffen.klass...@secunet.com>

Re: race condition in kernel/padata.c

[Steffen Klassert]

On Thu, Mar 23, 2017 at 12:03:43AM +0100, Jason A. Donenfeld wrote:
> Hey Steffen,
> 
> WireGuard makes really heavy use of padata, feeding it units of work
> from different cores in different contexts all at the same time. For
> the most part, everything has been fine, but one particular user has
> consistently run into mysterious bugs. He's using a rather old dual
> core CPU, which have a tendency to bring out race conditions
> sometimes. After struggling with getting a good backtrace, we finally
> managed to extract this from list debugging:
> 
> [87487.298728] WARNING: CPU: 1 PID: 882 at lib/list_debug.c:33
> __list_add+0xae/0x130
> [87487.301868] list_add corruption. prev->next should be next
> (b17abfc043d0), but was 8dba70872c80. (prev=8dba70872b00).
> [87487.339011]  [] dump_stack+0x68/0xa3
> [87487.342198]  [] ? console_unlock+0x281/0x6d0
> [87487.345364]  [] __warn+0xff/0x140
> [87487.348513]  [] warn_slowpath_fmt+0x4a/0x50
> [87487.351659]  [] __list_add+0xae/0x130
> [87487.354772]  [] ? _raw_spin_lock+0x64/0x70
> [87487.357915]  [] padata_reorder+0x1e6/0x420
> [87487.361084]  [] padata_do_serial+0xa5/0x120
> 
> padata_reorder calls list_add_tail with the list to which its adding
> locked, which seems correct:
> 
> spin_lock(>serial.lock);
> list_add_tail(>list, >serial.list);
> spin_unlock(>serial.lock);
> 
> This therefore leaves only place where such inconsistency could occur:
> if padata->list is added at the same time on two different threads.
> This pdata pointer comes from the function call to
> padata_get_next(pd), which has in it the following block:
> 
> next_queue = per_cpu_ptr(pd->pqueue, cpu);
> padata = NULL;
> reorder = _queue->reorder;
> if (!list_empty(>list)) {
>padata = list_entry(reorder->list.next,
>struct padata_priv, list);
>spin_lock(>lock);
>list_del_init(>list);
>atomic_dec(>reorder_objects);
>spin_unlock(>lock);
> 
>pd->processed++;
> 
>goto out;
> }
> out:
> return padata;
> 
> I strongly suspect that the problem here is that two threads can race
> on reorder list. Even though the deletion is locked, call to
> list_entry is not locked, which means it's feasible that two threads
> pick up the same padata object and subsequently call list_add_tail on
> them at the same time. The fix would thus be to hoist that lock
> outside of that block.

Yes, looks like we should lock the whole list handling block here.

Thanks!

Re: [PATCH] padata: Remove unused but set variables

[Steffen Klassert]

On Mon, Oct 17, 2016 at 12:16:08PM +0200, Tobias Klauser wrote:
> Remove the unused but set variable pinst in padata_parallel_worker to
> fix the following warning when building with 'W=1':
> 
>   kernel/padata.c: In function ‘padata_parallel_worker’:
>   kernel/padata.c:68:26: warning: variable ‘pinst’ set but not used 
> [-Wunused-but-set-variable]
> 
> Also remove the now unused variable pd which is only used to set pinst.
> 
> Signed-off-by: Tobias Klauser <tklau...@distanz.ch>

Acked-by: Steffen Klassert <steffen.klass...@secunet.com>

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] padata: add helper function for queue length

[Steffen Klassert]

On Sun, Oct 02, 2016 at 03:46:38AM +0200, Jason A. Donenfeld wrote:
> Since padata has a maximum number of inflight jobs, currently 1000, it's
> very useful to know how many jobs are currently queued up. This adds a
> simple helper function to expose this information.
> 
> Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
> ---
>  include/linux/padata.h |  2 ++
>  kernel/padata.c| 16 
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/linux/padata.h b/include/linux/padata.h
> index 113ee62..4840ae4 100644
> --- a/include/linux/padata.h
> +++ b/include/linux/padata.h
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2008, 2009 secunet Security Networks AG
>   * Copyright (C) 2008, 2009 Steffen Klassert <steffen.klass...@secunet.com>
> + * Copyright (C) 2016 Jason A. Donenfeld <ja...@zx2c4.com>
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -181,4 +182,5 @@ extern int padata_register_cpumask_notifier(struct 
> padata_instance *pinst,
>   struct notifier_block *nblock);
>  extern int padata_unregister_cpumask_notifier(struct padata_instance *pinst,
> struct notifier_block *nblock);
> +extern int padata_queue_len(struct padata_instance *pinst);
>  #endif
> diff --git a/kernel/padata.c b/kernel/padata.c
> index 9932788..17c1e08 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -5,6 +5,7 @@
>   *
>   * Copyright (C) 2008, 2009 secunet Security Networks AG
>   * Copyright (C) 2008, 2009 Steffen Klassert <steffen.klass...@secunet.com>
> + * Copyright (C) 2016 Jason A. Donenfeld <ja...@zx2c4.com>
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms and conditions of the GNU General Public License,
> @@ -1039,3 +1040,18 @@ void padata_free(struct padata_instance *pinst)
>   kobject_put(>kobj);
>  }
>  EXPORT_SYMBOL(padata_free);
> +
> +/**
> + * padata_queue_len - retreive the number of in progress jobs
> + *
> + * @padata_inst: padata instance from which to read the queue size
> + */
> +int padata_queue_len(struct padata_instance *pinst)
> +{
> + int len;
> + rcu_read_lock_bh();
> + len = atomic_read(_dereference_bh(pinst->pd)->refcnt);
> + rcu_read_unlock_bh();
> + return len;
> +}
> +EXPORT_SYMBOL(padata_queue_len);

Why you want to have this? Without having a user of this function,
there is no point on adding it.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] crypto: Fix IPsec reordering caused by cryptd

[Steffen Klassert]

On Tue, Jun 21, 2016 at 04:53:21PM +0800, Herbert Xu wrote:
> Hi:
> 
> I finally got around to working on this.  I quickly gave up on the
> notion of hijacking the queued requests as we may end up overwhelming
> our caller.
> 
> So the solution is the obvious one of using cryptd as long as there
> are requests queued there belonging to the same tfm.  This is
> totally untested so please tread carefully.

Thanks for the patches!
I've tested them today and observed no problems with it,
so looks good :)

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: padata - is serial actually serial?

[Steffen Klassert]

Hi Jason.

On Tue, Jun 14, 2016 at 11:00:54PM +0200, Jason A. Donenfeld wrote:
> Hi Steffen & Folks,
> 
> I submit a job to padata_do_parallel(). When the parallel() function
> triggers, I do some things, and then call padata_do_serial(). Finally
> the serial() function triggers, where I complete the job (check a
> nonce, etc).
> 
> The padata API is very appealing because not only does it allow for
> parallel computation, but it claims that the serial() functions will
> execute in the order that jobs were originally submitted to
> padata_do_parallel().
> 
> Unfortunately, in practice, I'm pretty sure I'm seeing deviations from
> this. When I submit tons and tons of tasks at rapid speed to
> padata_do_parallel(), it seems like the serial() function isn't being
> called in the exactly the same order that tasks were submitted to
> padata_do_parallel().
> 
> Is this known (expected) behavior? Or have I stumbled upon a potential
> bug that's worthwhile for me to investigate more?

It should return in the same order as the job were submitted,
given that the submitting cpu and the callback cpu are fixed
for all the jobs you want to preserve the order.  If you submit
jobs from more than one cpu, we can not know in which order
they are enqueued. The cpu that gets the lock as the first
has its job in front. Same if you use more than one callback cpu
we can't know in which order they are dequeued, because the
serial workers are scheduled independent on each cpu.

I use it in crypto/pcrypt.c and I never had problems.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] crypto: hash - Fix page length clamping in hash walk

[Steffen Klassert]

On Wed, May 04, 2016 at 05:52:56PM +0800, Herbert Xu wrote:
> On Wed, May 04, 2016 at 11:34:20AM +0200, Steffen Klassert wrote:
> > 
> > Hmm, the 'sleeping while atomic' because of not unmapping
> > the page goes away, but now I see a lot of IPsec ICV fails
> > on the receive side. I'll try to find out what's going on.
> > 
> > Sowmini, could you please doublecheck with your test setup?
> 
> Don't bother, my patch was crap.  Here's one that's more likely
> to work:

This one is much better, works here :)

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crypto: hash - Fix page length clamping in hash walk

[Steffen Klassert]

On Tue, May 03, 2016 at 05:55:31PM +0800, Herbert Xu wrote:
> On Thu, Apr 28, 2016 at 10:27:43AM +0200, Steffen Klassert wrote:
> >
> > The problem was that if offset (in a superpage) equals
> > PAGE_SIZE in hash_walk_next(), nbytes becomes zero. So
> > we map the page, but we don't hash and unmap because we
> > exit the loop in shash_ahash_update() in this case.
> 
> I see.  Does this patch help?

Hmm, the 'sleeping while atomic' because of not unmapping
the page goes away, but now I see a lot of IPsec ICV fails
on the receive side. I'll try to find out what's going on.

Sowmini, could you please doublecheck with your test setup?

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] crypto: Make the page handling of hash walk compatible to networking.

[Steffen Klassert]

On Mon, Apr 25, 2016 at 06:05:27PM +0800, Herbert Xu wrote:
> On Thu, Apr 21, 2016 at 09:14:51AM +0200, Steffen Klassert wrote:
> > The network layer tries to allocate high order pages for skb_buff
> > fragments, this leads to problems if we pass such a buffer to
> > crypto because crypto assumes to have always order null pages
> > in the scatterlists.
> 
> I don't understand.  AFAICS the crypto API assumes no such thing.
> Of course there might be a bug there since we probably don't get
> too many superpages coming in normally.

Maybe I misinterpreted the things I observed.

> 
> > Herbert, I could not find out why this PAGE_SIZE limit is in place.
> > So not sure if this is the right fix. Also, would it be ok to merge
> > this, or whatever is the right fix through the IPsec tree? We need
> > this before we can change esp to avoid linearization.
> 
> Your patch makes no sense.  

That's possible :)

> When you do a kmap you can only do
> one page at a time.  So if you have a "superpage" (an SG list
> entry with multiple contiguous pages) then you must walk it one
> page at a time.
> 
> That's why we cap it at PAGE_SIZE.
> 
> Is it not walking the superpage properly?

The problem was that if offset (in a superpage) equals
PAGE_SIZE in hash_walk_next(), nbytes becomes zero. So
we map the page, but we don't hash and unmap because we
exit the loop in shash_ahash_update() in this case.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH] crypto: Make the page handling of hash walk compatible to networking.

[Steffen Klassert]

The network layer tries to allocate high order pages for skb_buff
fragments, this leads to problems if we pass such a buffer to
crypto because crypto assumes to have always order null pages
in the scatterlists.

This was not a problem so far, because the network stack linearized
all buffers before passing them to crypto. If we try to avoid the
linearization with skb_cow_data in IPsec esp4/esp6 this incompatibility
becomes visible.

Signed-off-by: Steffen Klassert <steffen.klass...@secunet.com>
---

Herbert, I could not find out why this PAGE_SIZE limit is in place.
So not sure if this is the right fix. Also, would it be ok to merge
this, or whatever is the right fix through the IPsec tree? We need
this before we can change esp to avoid linearization.

The full IPsec patchset can be found here:

https://git.kernel.org/cgit/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-ipsec-offload-work

 crypto/ahash.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/crypto/ahash.c b/crypto/ahash.c
index 5fc1f17..ca92783 100644
--- a/crypto/ahash.c
+++ b/crypto/ahash.c
@@ -44,8 +44,7 @@ static int hash_walk_next(struct crypto_hash_walk *walk)
 {
unsigned int alignmask = walk->alignmask;
unsigned int offset = walk->offset;
-   unsigned int nbytes = min(walk->entrylen,
- ((unsigned int)(PAGE_SIZE)) - offset);
+   unsigned int nbytes = walk->entrylen;
 
if (walk->flags & CRYPTO_ALG_ASYNC)
walk->data = kmap(walk->pg);
@@ -91,8 +90,6 @@ int crypto_hash_walk_done(struct crypto_hash_walk *walk, int 
err)
walk->offset = ALIGN(walk->offset, alignmask + 1);
walk->data += walk->offset;
 
-   nbytes = min(nbytes,
-((unsigned int)(PAGE_SIZE)) - walk->offset);
walk->entrylen -= nbytes;
 
return nbytes;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vti6: Add pmtu handling to vti6_xmit.

[Steffen Klassert]

On Wed, Mar 30, 2016 at 09:04:03PM +, Mark McKinstry wrote:
> I've tested this patch in our scenario and I can confirm that it still 
> fixes all of our issues.

I've applied the patch to the ipsec tree now.
Thanks for testing!
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vti6: Add pmtu handling to vti6_xmit.

[Steffen Klassert]

On Tue, Mar 15, 2016 at 01:28:01PM +0100, Steffen Klassert wrote:
> On Mon, Mar 14, 2016 at 09:52:05PM +, Mark McKinstry wrote:
> > Your patch adds a dst_release() call to my suggested fix, but this is 
> > problematic because the kfree_skb() call at tx_error already takes care 
> > of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() > 
> > skb_release_head_state() > skb_dst_drop()
> >  > refdst_drop() > dst_release(). In our scenario your patch results in 
> > a negative refcount kernel warning being generated in dst_release() for 
> > every packet that is too big to go over the vti.
> 
> Hm. I've just noticed that my pmtu test does not trigger this
> codepath, so I did not see the warning.
> 
> Seems like we do the pmtu handling too late, it should happen before
> we do skb_dst_set(). Also skb_scrub_packet() resets skb->ignore_df,
> so checking ignore_df after skb_scrub_packet() does not make much sense.
> 
> I'll send an updated version after some more testing.
> 

I've added a testcase that triggers this codepath to my testing
environment. The patch below works for me, could you please test
if it fixes your problems?

Subject: [PATCH] vti: Add pmtu handling to vti_xmit.

We currently rely on the PMTU discovery of xfrm.
However if a packet is locally sent, the PMTU mechanism
of xfrm tries to do local socket notification what
might not work for applications like ping that don't
check for this. So add pmtu handling to vti_xmit to
report MTU changes immediately.

Signed-off-by: Steffen Klassert <steffen.klass...@secunet.com>
---
 net/ipv4/ip_vti.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 5cf10b7..a917903 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -156,6 +156,7 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct 
net_device *dev,
struct dst_entry *dst = skb_dst(skb);
struct net_device *tdev;/* Device to other host */
int err;
+   int mtu;
 
if (!dst) {
dev->stats.tx_carrier_errors++;
@@ -192,6 +193,23 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct 
net_device *dev,
tunnel->err_count = 0;
}
 
+   mtu = dst_mtu(dst);
+   if (skb->len > mtu) {
+   skb_dst(skb)->ops->update_pmtu(skb_dst(skb), NULL, skb, mtu);
+   if (skb->protocol == htons(ETH_P_IP)) {
+   icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
+ htonl(mtu));
+   } else {
+   if (mtu < IPV6_MIN_MTU)
+   mtu = IPV6_MIN_MTU;
+
+   icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+   }
+
+   dst_release(dst);
+   goto tx_error;
+   }
+
skb_scrub_packet(skb, !net_eq(tunnel->net, dev_net(dev)));
skb_dst_set(skb, dst);
skb->dev = skb_dst(skb)->dev;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vti6: Add pmtu handling to vti6_xmit.

[Steffen Klassert]

On Mon, Mar 14, 2016 at 09:52:05PM +, Mark McKinstry wrote:
> Your patch adds a dst_release() call to my suggested fix, but this is 
> problematic because the kfree_skb() call at tx_error already takes care 
> of releasing dst - via kfree_skb() > __kfree_skb() > skb_release_all() > 
> skb_release_head_state() > skb_dst_drop()
>  > refdst_drop() > dst_release(). In our scenario your patch results in 
> a negative refcount kernel warning being generated in dst_release() for 
> every packet that is too big to go over the vti.

Hm. I've just noticed that my pmtu test does not trigger this
codepath, so I did not see the warning.

Seems like we do the pmtu handling too late, it should happen before
we do skb_dst_set(). Also skb_scrub_packet() resets skb->ignore_df,
so checking ignore_df after skb_scrub_packet() does not make much sense.

I'll send an updated version after some more testing.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vti6: Add pmtu handling to vti6_xmit.

[Steffen Klassert]

On Wed, Feb 24, 2016 at 09:37:39PM +, Mark McKinstry wrote:
> On 19/02/16 01:19, Steffen Klassert wrote:
> > On Thu, Feb 18, 2016 at 01:40:00AM +, Mark McKinstry wrote:
> >> This patch fixes our issue, thanks. In our scenario the tunnel path MTU
> >> now gets updated so that subsequent large packets sent over the tunnel
> >> get fragmented correctly.
> > I've applied this patch to the ipsec tree now.
> > Thanks for testing!
> I spoke too soon. Upon further testing with this patch we have found it 
> causes
> a skt buffer leak. This is problematic for us and can cause memory 
> exhaustion in
> one of our test scenarios that has an IPv4 IPsec tunnel over a PPP link. 

The patch below is what I plan to apply on top of the original patch.

Subject: [PATCH] vti: Fix recource leeks on pmtu discovery

A recent patch introduced pmtu handling directly in the
vti transmit routine. Unfortunately we now return without
releasing the dst_entry and freeing the sk_buff. This patch
fixes the issue.

Fixes: 325b71fe0f57 ("vti: Add pmtu handling to vti_xmit.")
Reported-by: Mark McKinstry <mark.mckins...@alliedtelesis.co.nz>
Signed-off-by: Steffen Klassert <steffen.klass...@secunet.com>
---
 net/ipv4/ip_vti.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
index 6862305..2ea2b6e 100644
--- a/net/ipv4/ip_vti.c
+++ b/net/ipv4/ip_vti.c
@@ -206,7 +206,8 @@ static netdev_tx_t vti_xmit(struct sk_buff *skb, struct 
net_device *dev,
else
icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
 
-   return -EMSGSIZE;
+   dst_release(dst);
+   goto tx_error;
}
 
err = dst_output(tunnel->net, skb->sk, skb);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vti6: Add pmtu handling to vti6_xmit.

[Steffen Klassert]

On Wed, Feb 24, 2016 at 09:37:39PM +, Mark McKinstry wrote:
> On 19/02/16 01:19, Steffen Klassert wrote:
> > On Thu, Feb 18, 2016 at 01:40:00AM +, Mark McKinstry wrote:
> >> This patch fixes our issue, thanks. In our scenario the tunnel path MTU
> >> now gets updated so that subsequent large packets sent over the tunnel
> >> get fragmented correctly.
> > I've applied this patch to the ipsec tree now.
> > Thanks for testing!
> I spoke too soon. Upon further testing with this patch we have found it 
> causes
> a skt buffer leak. This is problematic for us and can cause memory 
> exhaustion in
> one of our test scenarios that has an IPv4 IPsec tunnel over a PPP link. 
> Also
> the patch's -EMSGSIZE return value appears to be invalid because vti_xmit()
> should be returning a type netdev_tx_t (NETDEV_TX_OK etc). It looks to 
> me that
> this patch should really be doing a goto tx_error rather than doing an early
> return with -EMSGSIZE. This would result in the skt buffer being freed,
> NETDEV_TX_OK being returned (thus indicating vti_xmit() "took care of 
> packet"),
> and the tx_errors counter being incremented (which seems like a reasonable
> thing to do).

Yes, you are right here. 

> 
> I think the original IPv6 patch probably has the same issues, and could be
> causing a DOS attack vulnerability in recent Linux releases.

We need to fix both, ipv4 and ipv6. I'll care for it,
thanks for the report.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] vti6: Add pmtu handling to vti6_xmit.

[Steffen Klassert]

On Thu, Feb 18, 2016 at 01:40:00AM +, Mark McKinstry wrote:
> This patch fixes our issue, thanks. In our scenario the tunnel path MTU 
> now gets updated so that subsequent large packets sent over the tunnel 
> get fragmented correctly.

I've applied this patch to the ipsec tree now.
Thanks for testing!
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ipsec impact on performance

[Steffen Klassert]

On Mon, Dec 07, 2015 at 06:27:48AM -0500, Sowmini Varadhan wrote:
> On (12/07/15 09:40), Steffen Klassert wrote:
> > 
> > I've pushed it to
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-ipsec-offload
> > 
> > It is just example code, nothing that I would show usually.
> > But you asked for it, so here is it :)
> 
> that's fine, I dont expect more at this point, just want to 
> test-drive it, and see how it compares to my approach. 

Would be nice if you could share the results. Comments are
welcome too, of course.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ipsec impact on performance

[Steffen Klassert]

On Thu, Dec 03, 2015 at 06:38:20AM -0500, Sowmini Varadhan wrote:
> On (12/03/15 09:45), Steffen Klassert wrote:
> > pcrypt(echainiv(authenc(hmac(sha1-ssse3),cbc-aes-aesni)))
> > 
> > Result:
> > 
> > iperf -c 10.0.0.12 -t 60
> > 
> > Client connecting to 10.0.0.12, TCP port 5001
> > TCP window size: 45.0 KByte (default)
> > 
> > [  3] local 192.168.0.12 port 39380 connected with 10.0.0.12 port 5001
> > [ ID] Interval   Transfer Bandwidth
> > [  3]  0.0-60.0 sec  32.8 GBytes  4.70 Gbits/sec
> > 
> > I provide more informatios as soon as the code is available.
> 
> that's pretty good compared to the baseline. 

This is GRO in combination with a pcrypt parallelized
crypto algorithm, without the parallelization GRO/GSO
does not help because crypto is the bottleneck then.

> I'd like to try out our patches, when they are ready.

I've pushed it to

https://git.kernel.org/cgit/linux/kernel/git/klassert/linux-stk.git/log/?h=net-next-ipsec-offload

It is just example code, nothing that I would show usually.
But you asked for it, so here is it :)

The GRO part seems to work well, the GSO part is just a hack at the 
moment.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ipsec impact on performance

[Steffen Klassert]

On Wed, Dec 02, 2015 at 07:05:38AM -0500, Sowmini Varadhan wrote:
> On (12/02/15 07:53), Steffen Klassert wrote:
> > 
> > I'm currently working on a GRO/GSO codepath for IPsec too. The GRO part
> > works already. I decapsulate/decrypt the packets on layer2 with a esp GRO
> > callback function and reinject them into napi_gro_receive(). So in case
> > the decapsulated packet is TCP, GRO can aggregate big packets.
> 
> Would you be able to share your patch with me? I'd like to give that a try
> just to get preliminary numbers (and I could massage it as needed
> for transport mode too).

I've got the final bits to work today, I can do async crypto now.
I can push the patches to a public tree after some polishing.
But I have to warn, it has still bugs and no usefull commit messages.

I did a first test with forwaring esp in tunnel mode. The crypto
algorithm I used was:

pcrypt(echainiv(authenc(hmac(sha1-ssse3),cbc-aes-aesni)))

Result:

iperf -c 10.0.0.12 -t 60

Client connecting to 10.0.0.12, TCP port 5001
TCP window size: 45.0 KByte (default)

[  3] local 192.168.0.12 port 39380 connected with 10.0.0.12 port 5001
[ ID] Interval   Transfer Bandwidth
[  3]  0.0-60.0 sec  32.8 GBytes  4.70 Gbits/sec

I provide more informatios as soon as the code is available.

> 
> > Another thing, I thought about setting up an IPsec BoF/workshop at
> > netdev1.1. My main topic is GRO/GSO for IPsec. I'll send out a mail
> > to the list later this week to see if there is enough interest and
> > maybe some additional topics.
> 
> Sounds like an excellent idea. I'm certainly interested.

Great, than we are at least two :)
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ipsec impact on performance

[Steffen Klassert]

On Thu, Dec 03, 2015 at 06:38:20AM -0500, Sowmini Varadhan wrote:
> On (12/03/15 09:45), Steffen Klassert wrote:
> > pcrypt(echainiv(authenc(hmac(sha1-ssse3),cbc-aes-aesni)))
> > 
> > Result:
> > 
> > iperf -c 10.0.0.12 -t 60
> > 
> > Client connecting to 10.0.0.12, TCP port 5001
> > TCP window size: 45.0 KByte (default)
> > 
> > [  3] local 192.168.0.12 port 39380 connected with 10.0.0.12 port 5001
> > [ ID] Interval   Transfer Bandwidth
> > [  3]  0.0-60.0 sec  32.8 GBytes  4.70 Gbits/sec
> > 
> > I provide more informatios as soon as the code is available.
> 
> that's pretty good compared to the baseline. 
> I'd like to try out our patches, when they are ready.
> 
> I think you may get some more improvement if you manually pin the irq 
> and iperf to specific cpus (at least that was my observation for transp 
> mode) 

I do that already. I have dedicated crypto and IO cpus, 2 cpus
do networking IO and 4 cpus do crypto (parallelized with pcrypt).

The bottleneck is now the cpu that does the TX path (checksumming
of the GSO segments).

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ipsec impact on performance

[Steffen Klassert]

On Tue, Dec 01, 2015 at 12:59:53PM -0500, Sowmini Varadhan wrote:
> 
> I instrumented iperf with and without ipsec, just using esp-null, 
> and 1 thread, to keep things simple. I'm seeing some pretty dismal 
> performance numbers with ipsec, and trying to think of ways to
> improve this. Here are my findings, please share feedback.
> 
> I suspect that a big part of the problem is the implicit loss of GSO,
> and this is made worse by some inefficiencies in the xfrm code:
> for single stream iperf (to avoid effects of rx-hash), I see the
> following on a 10G p2p ethernet link.
>  8.5-9.5 Gbps clear traffic, TSO disabled, so GSO, GRO is in effect
>  3-4 Gbps clear traffic, with both TSO/GSO disabled
>  1.8-2 Gbps for esp-null.
> So the above numbers suggest that losing TSO/GSO results in one
> big drop in performance, and then there's another cliff for the 
> clear -> esp-null transition. And those cliffs apply even if you are
> merely doing TCP-MD5 or AO for basic protection of the TCP connection.
> 
> I tried moving things about a bit to defer the ipsec after GSO - I'll 
> share my experimental patch as an RFC in a separate thread. (Disclaimer:
> the patch is just an experiment at this point).
> 
> In that patch, I'm only focussing on esp-null and transp-mode ipsec
> for now, just to get some basic performance numbers to see if this is 
> at all interesting.  Essentially my hack mainly involves the following
> 
> - don't disable TSO in sk_setup_caps() if a dst->header_len is found
> - in xfrm4_output, if GSO is applicable, bail out without esp header 
>   addition - that will get done after skb_segment()
> - at the end of tcp_gso_segment() (when tcp segment is available),
>   set things up for xfrm_output_one and trigger the esp_output..
>   I have to be very careful about setting up skb pointers here, since
>   it looks like esp_output overloads the mac_header pointer e.g., for
>   setting up the ip protocol field 

I'm currently working on a GRO/GSO codepath for IPsec too. The GRO part
works already. I decapsulate/decrypt the packets on layer2 with a esp GRO
callback function and reinject them into napi_gro_receive(). So in case
the decapsulated packet is TCP, GRO can aggregate big packets.

My approach to GSO is a bit different to yours. I focused on tunnel mode,
but transport mode should work too. I encapsulate the big GSO packets
but don't do the encryption. Then I've added a esp_gso_segment() function,
so the (still not encrypted ESP packets) get segmented with GSO. Finally I
do encryption for all segments. This works well as long as I do sync crypto.
The hard part is when crypto returns async. This is what I'm working on now.
I hope to get this ready during the next weeks that I can post a RFC version
and some numbers.

Also I tried to consider the IPsec GRO/GSO codepath as a software fallback.
So I added hooks for the encapsulation, encryption etc. If a NIC can do
IPsec, it can use this hooks to prepare the packets the way it needs it.
There are NICs that can do IPsec, it's just that our stack does not support
it.

Another thing, I thought about setting up an IPsec BoF/workshop at
netdev1.1. My main topic is GRO/GSO for IPsec. I'll send out a mail
to the list later this week to see if there is enough interest and
maybe some additional topics.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 2/2] xfrm: Fix unaligned access in xfrm_notify_sa() for DELSA

[Steffen Klassert]

On Mon, Oct 19, 2015 at 05:23:29PM -0400, Sowmini Varadhan wrote:
> On sparc, deleting established SAs (e.g., by restarting ipsec
> at the peer) results in unaligned access messages via
> xfrm_del_sa -> km_state_notify -> xfrm_send_state_notify().
> Use an aligned pointer to xfrm_usersa_info for this case.
> 
> Signed-off-by: Sowmini Varadhan 
> ---
>  net/xfrm/xfrm_user.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index a8de9e3..158ef4a 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2659,7 +2659,7 @@ static int xfrm_notify_sa(struct xfrm_state *x, const 
> struct km_event *c)
>   if (attr == NULL)
>   goto out_free_skb;
>  
> - p = nla_data(attr);
> + p = PTR_ALIGN(nla_data(attr), __alignof__(*p));

Hm, this breaks userspace notifications on 64-bit systems.
Userspace expects this to be aligned to 4, with your patch
it is aligned to 8 on 64-bit.

Without your patch I get the correct notification when deleting a SA:

ip x m

Deleted src 172.16.0.2 dst 172.16.0.1
proto esp spi 0x0002 reqid 2 mode tunnel
replay-window 32
auth-trunc hmac(sha1) 0x31323334353637383930 96
enc cbc(aes) 0x31323334353637383930313233343536
sel src 10.0.0.0/24 dst 192.168.0.0/24

With your patch I get for the same SA:

ip x m

Deleted src 50.0.0.0 dst 0.0.0.0
proto 0 reqid 0 mode transport
replay-window 0 flag decap-dscp
auth-trunc hmac(sha1) 0x31323334353637383930 96
enc cbc(aes) 0x31323334353637383930313233343536
sel src 0.0.0.0/0 dst 0.234.255.255/0 proto igmp

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/13] ipsec: Replace seqniv with seqiv

[Steffen Klassert]

On Thu, Aug 13, 2015 at 05:28:52PM +0800, Herbert Xu wrote:
 Now that seqniv is identical with seqiv we no longer need it.
 
 Signed-off-by: Herbert Xu herb...@gondor.apana.org.au

Acked-by: Steffen Klassert steffen.klass...@secunet.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/9] crypto: Add ChaCha20-Poly1305 AEAD support for IPsec

[Steffen Klassert]

On Wed, Jun 03, 2015 at 10:44:25AM +0800, Herbert Xu wrote:
 On Mon, Jun 01, 2015 at 01:43:55PM +0200, Martin Willi wrote:
  This is a first version of a patch series implementing the ChaCha20-Poly1305
  AEAD construction defined in RFC7539. It is based on the current cryptodev 
  tree.
 
 The patches look fine to me.  Steffen, what do you think?

I'm fine with this. If you want to merge this series through the
cryptodev tree, feel free to add a

Acked-by: Steffen Klassert steffen.klass...@secunet.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfrm6: Do not use xfrm_local_error for path MTU issues in tunnels

[Steffen Klassert]

On Thu, May 28, 2015 at 12:18:51AM -0700, Alexander Duyck wrote:
 On 05/27/2015 10:36 PM, Steffen Klassert wrote:
 On Wed, May 27, 2015 at 10:40:32AM -0700, Alexander Duyck wrote:
 This change makes it so that we use icmpv6_send to report PMTU issues back
 into tunnels in the case that the resulting packet is larger than the MTU
 of the outgoing interface.  Previously xfrm_local_error was being used in
 this case, however this was resulting in no changes, I suspect due to the
 fact that the tunnel itself was being kept out of the loop.
 
 This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the
 behavior seen if the socket was orphaned.  Instead of requiring the socket
 to be orphaned this patch simply defaults to using icmpv6_send in the case
 that the frame came though a tunnel.
 We can use icmpv6_send() just in the case that the packet
 was already transmitted by a tunnel device, otherwise we
 get the bug back that I mentioned in my other mail.
 
 Not sure if we have something to know that the packet
 traversed a tunnel device. That's what I asked in the
 thread 'Looking for a lost patch'.
 
 Okay I will try to do some more digging.  From what I can tell right
 now it looks like my ping attempts are getting hung up on the
 xfrm_local_error in __xfrm6_output.  I wonder if we couldn't somehow
 make use of the skb-cb to store a pointer to the tunnel that could
 be checked to determine if we are going through a VTI or not.

Maybe it is as easy as the patch below, could you please test it?

Subject: [PATCH RFC] vti6: Add pmtu handling to vti6_xmit.

We currently rely on the PMTU discovery of xfrm.
However if a packet is localy sent, the PMTU mechanism
of xfrm tries to to local socket notification what
might not work for applications like ping that don't
check for this. So add pmtu handling to vti6_xmit to
report MTU changes immediately.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 net/ipv6/ip6_vti.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index ff3bd86..13cb771 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -434,6 +434,7 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, 
struct flowi *fl)
struct dst_entry *dst = skb_dst(skb);
struct net_device *tdev;
struct xfrm_state *x;
+   int mtu;
int err = -1;
 
if (!dst)
@@ -468,6 +469,15 @@ vti6_xmit(struct sk_buff *skb, struct net_device *dev, 
struct flowi *fl)
skb_dst_set(skb, dst);
skb-dev = skb_dst(skb)-dev;
 
+   mtu = dst_mtu(dst);
+   if (!skb-ignore_df  skb-len  mtu) {
+   skb_dst(skb)-ops-update_pmtu(dst, NULL, skb, mtu);
+
+   icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
+
+   return -EMSGSIZE;
+   }
+
err = dst_output(skb);
if (net_xmit_eval(err) == 0) {
struct pcpu_sw_netstats *tstats = this_cpu_ptr(dev-tstats);
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v3 PATCH 0/8] crypto: Convert all AEAD users to new interface

[Steffen Klassert]

On Wed, May 27, 2015 at 04:01:05PM +0800, Herbert Xu wrote:
 Hi:
 
 The only changes from the last version are that set_ad no longer
 takes a cryptoff argument and testmgr has been updated to always
 supply space for the authentication tag.
 
 The algif_aead patch has been removed and will be posted separately.
 
 Series description:
 
 This series of patches convert all in-tree AEAD users that I
 could find to the new single SG list interface.  For IPsec it
 also adopts the new explicit IV generator scheme.
 
 To recap, the old AEAD interface takes an associated data (AD)
 SG list in addition to the plain/cipher text SG list(s).  That
 forces the underlying AEAD algorithm implementors to try to stitch
 those two lists together where possible in order to maximise the
 contiguous chunk of memory passed to the ICV/hash function.  Things
 get even more hairy for IPsec as it has a third piece of memory,
 the generated IV (giv) that needs to be hashed.  One look at the
 nasty things authenc does for example is enough to make anyone
 puke :)
 
 In fact the interface is just getting in our way because for the
 main user IPsec the data is naturally contiguous as the protocol
 was designed with this in mind.
 
 So the new AEAD interface gets rid of the separate AD SG list
 and instead simply requires the AD to be at the head of the src
 and dst SG lists.
 
 The conversion of in-tree users is fairly straightforward.  The
 only non-trivial bit is IPsec as I'm taking this opportunity to
 move the IV generation knowledge into IPsec as that's where it
 belongs since we may in future wish to support different generation
 schemes for a single algorithm.

Not sure if I missed something in the flood of patches, but if I
apply your v3 patchset on top of the cryptodev tree, it crashes
like that buring boot:

[4.668297] [ cut here ]
[4.669143] kernel BUG at 
/home/klassert/git/linux-stk/include/linux/scatterlist.h:67!
[4.670457] invalid opcode:  [#1] SMP DEBUG_PAGEALLOC
[4.671595] CPU: 0 PID: 1363 Comm: cryptomgr_test Not tainted 4.0.0+ #951
[4.672025] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
Bochs 01/01/2011
[4.672025] task: ce9e7300 ti: ceb54000 task.ti: ceb54000
[4.672025] EIP: 0060:[c11d45b5] EFLAGS: 00010206 CPU: 0
[4.672025] EIP is at scatterwalk_ffwd+0xf5/0x100
[4.672025] EAX: ceb43b20 EBX: ceb55c94 ECX: 0014 EDX: c11db23f
[4.672025] ESI: 0010 EDI: 0003 EBP: ceb55c7c ESP: ceb55c6c
[4.672025]  DS: 007b ES: 007b FS: 00d8 GS:  SS: 0068
[4.672025] CR0: 8005003b CR2: bfbb6fc0 CR3: 0eb26000 CR4: 06d0
[4.672025] Stack:
[4.672025]  cffd28c0 0014 ceb35400 cea33618 ceb55cd0 c11d45e8 ceb43b20 

[4.672025]  ceb35438 c11db220 ceb55c9c c11db23f ceb55cac c11da470 ceb35438 
ceb353c8
[4.672025]  ceb55cb4 c11da763 ceb55cd0 c11f2c6f ceb35400 0200 ceb35358 
ceb353c8
[4.672025] Call Trace:
[4.672025]  [c11d45e8] scatterwalk_map_and_copy+0x28/0xc0
[4.672025]  [c11db220] ? shash_ahash_finup+0x80/0x80
[4.672025]  [c11db23f] ? shash_async_finup+0x1f/0x30
[4.672025]  [c11da470] ? crypto_ahash_op+0x20/0x50
[4.672025]  [c11da763] ? crypto_ahash_finup+0x13/0x20
[4.672025]  [c11f2c6f] ? crypto_authenc_ahash_fb+0xaf/0xd0
[4.672025]  [c11f2dfc] crypto_authenc_genicv+0xfc/0x340
[4.672025]  [c11f3526] crypto_authenc_encrypt+0x96/0xb0
[4.672025]  [c11f3490] ? crypto_authenc_decrypt+0x3e0/0x3e0
[4.672025]  [c11d4eb7] old_crypt+0xa7/0xc0
[4.672025]  [c11d4f09] old_encrypt+0x19/0x20
[4.672025]  [c11ddbe8] __test_aead+0x268/0x1580
[4.672025]  [c11d28a7] ? __crypto_alloc_tfm+0x37/0x120
[4.672025]  [c11d28a7] ? __crypto_alloc_tfm+0x37/0x120
[4.672025]  [c11d7742] ? skcipher_geniv_init+0x22/0x40
[4.672025]  [c11d7d73] ? eseqiv_init+0x43/0x50
[4.672025]  [c11d2936] ? __crypto_alloc_tfm+0xc6/0x120
[4.672025]  [c11df101] test_aead+0x31/0xc0
[4.672025]  [c11df1d3] alg_test_aead+0x43/0xa0
[4.672025]  [c11def2e] ? alg_find_test+0x2e/0x70
[4.672025]  [c11dfe42] alg_test+0xa2/0x240
[4.672025]  [c106dd83] ? finish_task_switch+0x83/0xe0
[4.672025]  [c159c002] ? __schedule+0x412/0x1067
[4.672025]  [c1085f57] ? __wake_up_common+0x47/0x70
[4.672025]  [c11dbc10] ? cryptomgr_notify+0x450/0x450
[4.672025]  [c11dbc4f] cryptomgr_test+0x3f/0x50
[4.672025]  [c1066dfb] kthread+0xab/0xc0
[4.672025]  [c15a1a41] ret_from_kernel_thread+0x21/0x30
[4.672025]  [c1066d50] ? __kthread_parkme+0x80/0x80
[4.672025] Code: 83 c4 04 5b 5e 5f 5d c3 81 3b 21 43 65 87 75 13 8b 43 04 
83 e0 fe 83 c8 02 89 43 04 89 d8 e9 4d ff ff ff 0f 0b 0f 0b 0f 0b 0f 0b 0f 0b 
0f 0b 8d b4 26 00 00 00 00 55 89 e5 57 56 53 83 ec 40 3e
[4.672025] EIP: [c11d45b5] scatterwalk_ffwd+0xf5/0x100 SS:ESP 
0068:ceb55c6c
[4.721562] ---[ end trace 94a02f0816fe7c7f ]---

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body 

Re: [v3 PATCH 0/8] crypto: Convert all AEAD users to new interface

[Steffen Klassert]

On Wed, May 27, 2015 at 05:29:22PM +0800, Herbert Xu wrote:
 On Wed, May 27, 2015 at 11:25:33AM +0200, Steffen Klassert wrote:
  
  Not sure if I missed something in the flood of patches, but if I
  apply your v3 patchset on top of the cryptodev tree, it crashes
  like that buring boot:
 
 Sorry, I forgot to mention that v3 depends on the series of fixes
 posted just before it (but only to linux-crypto):
 
 https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg14487.html
 

OK, I'll try with this.

Thanks!

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ipsec PATCH 0/3] Preserve skb-mark through VTI tunnels

[Steffen Klassert]

On Wed, May 27, 2015 at 07:16:37AM -0700, Alexander Duyck wrote:
 These patches are meant to try and address the fact the VTI tunnels are
 currently overwriting the skb-mark value.  I am generally happy with the
 first two patches, however the third patch still modifies the skb-mark,
 though it undoes after the fact.
 
 The main problem I am trying to address is the fact that currently if I use
 an v6 over v6 VTI tunnel I cannot receive any traffic on the interface as
 the skb-mark is bleeding through and causing the traffic to be dropped.
 
 ---
 
 Alexander Duyck (3):
   ip_vti/ip6_vti: Do not touch skb-mark on xmit
   xfrm: Override skb-mark with tunnel-parm.i_key in xfrm_input
   ip_vti/ip6_vti: Preserve skb-mark after rcv_cb call

All applied to the ipsec tree, thanks a lot Alexander!
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfrm6: Do not use xfrm_local_error for path MTU issues in tunnels

[Steffen Klassert]

On Thu, May 28, 2015 at 12:49:19PM +0800, Herbert Xu wrote:
 On Wed, May 27, 2015 at 10:40:32AM -0700, Alexander Duyck wrote:
  This change makes it so that we use icmpv6_send to report PMTU issues back
  into tunnels in the case that the resulting packet is larger than the MTU
  of the outgoing interface.  Previously xfrm_local_error was being used in
  this case, however this was resulting in no changes, I suspect due to the
  fact that the tunnel itself was being kept out of the loop.
  
  This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the
  behavior seen if the socket was orphaned.  Instead of requiring the socket
  to be orphaned this patch simply defaults to using icmpv6_send in the case
  that the frame came though a tunnel.
  
  Signed-off-by: Alexander Duyck alexander.h.du...@redhat.com
 
 Does this still work with normal tunnel mode and identical inner
 and outer addresses? I recall we used to have a bug where in that
 situation the kernel would interpret the ICMP message as a reduction
 in outer MTU and thus resulting in a loop where the MTU keeps
 getting smaller.

Right, I think this reintroduces a bug that I fixed some years ago with
commit dd767856a36e (xfrm6: Don't call icmpv6_send on local error)
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfrm6: Do not use xfrm_local_error for path MTU issues in tunnels

[Steffen Klassert]

On Wed, May 27, 2015 at 10:40:32AM -0700, Alexander Duyck wrote:
 This change makes it so that we use icmpv6_send to report PMTU issues back
 into tunnels in the case that the resulting packet is larger than the MTU
 of the outgoing interface.  Previously xfrm_local_error was being used in
 this case, however this was resulting in no changes, I suspect due to the
 fact that the tunnel itself was being kept out of the loop.
 
 This patch fixes PMTU problems seen on ip6_vti tunnels and is based on the
 behavior seen if the socket was orphaned.  Instead of requiring the socket
 to be orphaned this patch simply defaults to using icmpv6_send in the case
 that the frame came though a tunnel.

We can use icmpv6_send() just in the case that the packet
was already transmitted by a tunnel device, otherwise we
get the bug back that I mentioned in my other mail.

Not sure if we have something to know that the packet
traversed a tunnel device. That's what I asked in the
thread 'Looking for a lost patch'.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [net-next PATCH RFC 0/3] Preserve skb-mark through VTI tunnels

[Steffen Klassert]

On Tue, May 26, 2015 at 03:41:10PM -0700, Alexander Duyck wrote:
 These patches are meant to try and address the fact the VTI tunnels are
 currently overwriting the skb-mark value.  I am generally happy with the
 first two patches, however the third patch still modifies the skb-mark,
 though it undoes after the fact.

I don't see any better solution, so I think this should be ok for now.
On the long run we need to replace this gre key/mark matching with
a separate interface.

 
 The main problem I am trying to address is the fact that currently if I use
 an v6 over v6 VTI tunnel I cannot receive any traffic on the interface as
 the skb-mark is bleeding through and causing the traffic to be dropped.

This is broken in the current mainline, so it should go into the ipsec
tree as a bugfix. I'd merge this patchset if you submit it to that tree.

Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CCM/GCM implementation defect

[Steffen Klassert]

On Thu, Apr 23, 2015 at 11:26:20AM +0800, Herbert Xu wrote:
 Hi:
 
 It looks like our IPsec implementations of CCM and GCM are buggy
 in that they don't include the IV in the authentication calculation.

Seems like crypto_rfc4106_crypt() passes the associated data it
got from ESP directly to gcm, without chaining with the IV.

 
 This definitely breaks interoperability with anyone who implements
 them correctly.  The fact that there have been no reports on this
 probably means that nobody has run into this in the field yet.
 
 On the security side, this is probably not a big deal for CCM
 because it always verifies the authentication tag after decryption.
 But for GCM this may be a DoS issue as an attacker could modify
 the IV without triggering the authentication check and thus cause
 an unnecessary decryption.  For both CCM and GCM this will result
 in random data injected as a packet into the network stack which
 hopefully will be dropped.
 
 In order to fix this without breaking backwards compatibility,
 my plan is to introduce new templates such as rfc4106v2 which
 implement the RFC correctly.  The existing templates will be
 retained so that current users aren't broken by the fix.

Adding a second template for the correct implementation is
probaply the only thing that we can do if we don't want to
break backwards compatibility. But maybe we can add a warning
to the old implementation, such that users notice that they
use a broken version.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Thu, Nov 20, 2014 at 03:43:42PM +0800, Herbert Xu wrote:
 On Thu, Nov 20, 2014 at 08:26:51AM +0100, Steffen Klassert wrote:
 
  What about to use a fallback algorithm that does not need to touch
  FPU/SIMD in such cases? We would not need cryptd at all and it would
  keep the requests in the right order because we don't defer them.
 
 This would be bad for throughput since the fallback is many orders
 of magnitude slower than aesni.

Sure, but could be an option if this is really a rare case.

Anyway, I don't mind too much about the solution as long as we
get it to work :)
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crypto: user - Allow get request with empty driver name

[Steffen Klassert]

On Thu, Nov 20, 2014 at 03:45:26PM +0800, Herbert Xu wrote:
 On Thu, Nov 20, 2014 at 08:11:42AM +0100, Steffen Klassert wrote:
 
  I think this is not sufficient, crypto_alg_match() will now return the first
  algorithm in crypto_alg_list that matches cra_name. We would need to extend
  crypto_alg_match() to return the algorithm with the highest priority in that
  case.
 
 It doesn't really matter because all implementations must provide
 exactly the same set of parameters for a given algorithm so it's
 good enough for what Stephan wants to do.

Ok, I see.

 But yes an interface to grab the highest priority algorithm would
 be useful too so patches are welcome :)

Should be not too hard to return the highest priority algorithm
instead of the first match with the existing interface, I'll see
what I can do.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/10] crypto: AF_ALG: user space interface for cipher info

[Steffen Klassert]

On Thu, Nov 20, 2014 at 05:03:24AM +0100, Stephan Mueller wrote:
 Am Dienstag, 18. November 2014, 22:08:23 schrieb Herbert Xu:
 
 Hi Herbert, Steffen,
 
  
  We already have crypto_user so you should be extending that to
  cover what's missing.
 
 After playing a bit with the interface, I think it falls short supporting 
 AF_ALG in the following way:
 
 crypto_user cannot be applied to the currently active cipher that one has 
 open 
 with AF_ALG. For getting information, one has to call crypto_user with the 
 cra_driver_name of a cipher. (Why is that limitation, btw (see crypto_report 
 and the use of cru_driver_name?)

crypto_report() was intended to provide informations of one implementation
of a algorithm, so it was required to specify this algorithm exactly with
cru_driver_name.

We could extend crypto_report() to provide informations of the algorithm
with the highest priority that matches cra_name.

Or, we also have crypto_dump_report(). This basically provides informations
on all instantiated algorithms, similar to /proc/crypto. We could extend
this in a way that you can provide a cra_name. Then it can dump out the
informations of all algorithms that match cra_name.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crypto: user - Allow get request with empty driver name

[Steffen Klassert]

On Thu, Nov 20, 2014 at 12:46:50PM +0800, Herbert Xu wrote:
 On Thu, Nov 20, 2014 at 05:23:23AM +0100, Stephan Mueller wrote:
  
  Here is the code:
  
  static int crypto_report(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
   struct nlattr **attrs)
  {
  ...
  if (!p-cru_driver_name[0])
  return -EINVAL;
 
 OK let's fix this.
 
 crypto: user - Allow get request with empty driver name
 
 Currently all get requests with an empty driver name fail with
 EINVAL.  Since most users actually want to supply an empty driver
 name this patch removes this check.
 
 Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
 
 diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
 index e2a34fe..0bb30ac 100644
 --- a/crypto/crypto_user.c
 +++ b/crypto/crypto_user.c
 @@ -201,10 +201,7 @@ static int crypto_report(struct sk_buff *in_skb, struct 
 nlmsghdr *in_nlh,
   if (!null_terminated(p-cru_name) || 
 !null_terminated(p-cru_driver_name))
   return -EINVAL;
  
 - if (!p-cru_driver_name[0])
 - return -EINVAL;
 -
 - alg = crypto_alg_match(p, 1);
 + alg = crypto_alg_match(p, 0);

I think this is not sufficient, crypto_alg_match() will now return the first
algorithm in crypto_alg_list that matches cra_name. We would need to extend
crypto_alg_match() to return the algorithm with the highest priority in that
case.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Sat, Nov 15, 2014 at 11:15:50AM +0800, Herbert Xu wrote:
 On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
 
  Everything below the local_bh_enable() should not run in atomic context
  as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.
 
 Actually I'm thinking of doing exactly that (disabling softirq in
 cryptd) to fix the reordering problem.
 
 Most threads do not use the FPU/SIMD so cryptd is only ever needed
 when we have a user-space app that touches the FPU/SIMD which then
 gets an interrupt to perform crypto in softirq.  So forcing cryptd
 on everyone just because some apps touch the FPU/SIMD is a non-
 starter.
 
 The most straightforward solution is to always defer to cryptd once
 it gets started.  This is bad because if a rarely used app that
 touches FPU/SIMD runs then we'll end up stuck in cryptd long after
 the app goes away.
 
 So what I'm thinking of is to have the softirq path forcibly regain
 control from cryptd where possible.  This is tricky because cryptd
 might be in the middle of processing a request.  So that's why I'd
 like to disable softirqs while we're processing a request.
 

What about to use a fallback algorithm that does not need to touch
FPU/SIMD in such cases? We would not need cryptd at all and it would
keep the requests in the right order because we don't defer them.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 01:49:31PM +0800, Ming Liu wrote:
 So far, the encryption/decryption are asynchronously processed in
 softirq and cryptd which would result in a implicit order of data,
 therefore leads IPSec stack also out of order while encapsulating
 or decapsulating packets.
 
 Consider the following scenario:
 
  DECRYPTION INBOUND
   |
   |
  +-Packet A
  ||
  ||
  | Packet B
  ||
 (cryptd) || (software interrupts)
  |  ..
  ||
  ||
  | Packet B(decrypted)
  ||
  ||
  +--- Packet A(decrypted)
   |
   |
  DECRYPTION OUTBOUND
 
 Add cryptd_flush_queue function fixing it by being called from softirq
 to flush all previous queued elements before processing a new one. To
 prevent cryptd_flush_queue() being accessed from software interrupts,
 local_bh_disable/enable needs to be relocated in several places.
 
 Signed-off-by: Ming Liu ming@windriver.com
 ---
 I was told that I need resend this patch with git, so here it is:
 
 I've figured out a new patch for this issue reported by me previously,
 the basic idea is adding a cryptd_flush_queue function fixing it by
 being called from softirq to flush all previous queued elements
 before processing a new one, and it works very well so far per my test.

I doubt that this approach can work. The cryptd encrypt/decrypt functions
assume to be called from a workqueue worker, they can't be called from
atomic context.

Can't we just use cryptd unconditionally to fix this reordering problem?

 
  crypto/ablk_helper.c|  10 -
  crypto/cryptd.c | 107 
 
  include/crypto/cryptd.h |  13 ++
  3 files changed, 111 insertions(+), 19 deletions(-)
 
 diff --git a/crypto/ablk_helper.c b/crypto/ablk_helper.c
 index ffe7278..600a70f 100644
 --- a/crypto/ablk_helper.c
 +++ b/crypto/ablk_helper.c
 @@ -70,16 +70,19 @@ int ablk_encrypt(struct ablkcipher_request *req)
  {
   struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
   struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
 + struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
 + crypto_ablkcipher_crt(ctx-cryptd_tfm-base)-base);
  
   if (!may_use_simd()) {
   struct ablkcipher_request *cryptd_req =
   ablkcipher_request_ctx(req);
  
   *cryptd_req = *req;
 - ablkcipher_request_set_tfm(cryptd_req, ctx-cryptd_tfm-base);
 + cryptd_req-base.tfm = req_tfm;
  
   return crypto_ablkcipher_encrypt(cryptd_req);
   } else {
 + cryptd_flush_queue(req_tfm, CRYPTD_ENCRYPT);

Where is CRYPTD_ENCRYPT defined?
This does not even compile when applied to the cryptodev tree.

   return __ablk_encrypt(req);
   }
  }
 @@ -89,13 +92,15 @@ int ablk_decrypt(struct ablkcipher_request *req)
  {
   struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
   struct async_helper_ctx *ctx = crypto_ablkcipher_ctx(tfm);
 + struct crypto_tfm *req_tfm = crypto_ablkcipher_tfm(
 + crypto_ablkcipher_crt(ctx-cryptd_tfm-base)-base);
  
   if (!may_use_simd()) {
   struct ablkcipher_request *cryptd_req =
   ablkcipher_request_ctx(req);
  
   *cryptd_req = *req;
 - ablkcipher_request_set_tfm(cryptd_req, ctx-cryptd_tfm-base);
 + cryptd_req-base.tfm = req_tfm;
  
   return crypto_ablkcipher_decrypt(cryptd_req);
   } else {
 @@ -105,6 +110,7 @@ int ablk_decrypt(struct ablkcipher_request *req)
   desc.info = req-info;
   desc.flags = 0;
  
 + cryptd_flush_queue(req_tfm, CRYPTD_DECRYPT);

Same here.

   return crypto_blkcipher_crt(desc.tfm)-decrypt(
   desc, req-dst, req-src, req-nbytes);
   }
 diff --git a/crypto/cryptd.c b/crypto/cryptd.c
 index e592c90..0b387a1 100644
 --- a/crypto/cryptd.c
 +++ b/crypto/cryptd.c
 @@ -119,11 +119,13 @@ static int cryptd_enqueue_request(struct cryptd_queue 
 *queue,
   int cpu, err;
   struct cryptd_cpu_queue *cpu_queue;
  
 + local_bh_disable();
   cpu = get_cpu();
   cpu_queue = this_cpu_ptr(queue-cpu_queue);
   err = crypto_enqueue_request(cpu_queue-queue, request);
   queue_work_on(cpu, kcrypto_wq, cpu_queue-work);
   put_cpu();
 + local_bh_enable();
  
   return err;
  }
 @@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct *work)
   preempt_disable();
   backlog = crypto_get_backlog(cpu_queue-queue);
   req = crypto_dequeue_request(cpu_queue-queue);
 - preempt_enable();
 - local_bh_enable();

Everything 

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 04:51:48PM +0800, Herbert Xu wrote:
 On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
 
  Can't we just use cryptd unconditionally to fix this reordering problem?
 
 I think the idea is that most of the time cryptd isn't required
 so we want to stick with direct processing to lower latency.

Yes, I thought that. But is it really the case that doing it
asynchronous increases the latency? I tried this some time
ago and as far as I remember there was not too much difference
between the direct and the asynchronous case. Might depend
on the usecase of course.

 
 I think the simplest fix would be to punt to cryptd as long as
 there are cryptd requests queued.

This would be the second option. We may need to adjust the
maximum cryptd queue lenght then, as the networking receive
softirq might enqueue a lot of packets before cryptd can
process them.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 06:41:30PM +0800, Ming Liu wrote:
 On 11/12/2014 04:51 PM, Herbert Xu wrote:
 On Wed, Nov 12, 2014 at 09:41:38AM +0100, Steffen Klassert wrote:
 Can't we just use cryptd unconditionally to fix this reordering problem?
 I think the idea is that most of the time cryptd isn't required
 so we want to stick with direct processing to lower latency.
 
 I think the simplest fix would be to punt to cryptd as long as
 there are cryptd requests queued.
 I've tried that method when I started to think about the fix, but it
 will cause 2 other issues per test while resolving the reordering
 one, as follows:
 1 The work queue can not handle so many packets when the traffic is
 very high(over 200M/S), and it would drop most of them when the
 queue length is beyond CRYPTD_MAX_CPU_QLEN.

That's why I've proposed to adjust CRYPTD_MAX_CPU_QLEN in my other mail.
But anyway, it still does not fix the reorder problem completely.
We still have a problem if subsequent algorithms run asynchronously
or if we get interrupted while we are processing the last request
from the queue.

I think we have only two options, either processing all calls
directly or use cryptd unconditionally. Mixing direct and
asynchronous calls will lead to problems.

If we don't want to use cryptd unconditionally, we could use
direct calls for all requests. If the fpu is not usable, we
maybe could fallback to an algorithm that does not need the
fpu, such as aes-generic.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: aesni-intel - avoid IPsec re-ordering

[Steffen Klassert]

On Wed, Nov 12, 2014 at 06:41:28PM +0800, Ming Liu wrote:
 On 11/12/2014 04:41 PM, Steffen Klassert wrote:
 On Wed, Nov 12, 2014 at 01:49:31PM +0800, Ming Liu wrote:
   }
 @@ -147,11 +149,9 @@ static void cryptd_queue_worker(struct work_struct 
 *work)
 preempt_disable();
 backlog = crypto_get_backlog(cpu_queue-queue);
 req = crypto_dequeue_request(cpu_queue-queue);
 -   preempt_enable();
 -   local_bh_enable();
 Everything below the local_bh_enable() should not run in atomic context
 as the subsequent functions may set the CRYPTO_TFM_REQ_MAY_SLEEP flag.
 If I turn off all the CRYPTO_TFM_REQ_MAY_SLEEP in cryptd.c, is that
 going to work?

Well, this might make the cryptd function accessible in atomic context,
but it does not solve the other problems with this approach. Also,
cryptd can be used to move requests out of atomic context and I think
it should stay as it is.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] pcrypt/padata rcu fixes

[Steffen Klassert]

On Thu, Nov 28, 2013 at 07:20:03PM +0100, Mathias Krause wrote:
 Two small RCU related fixes, lockdep complained about.
 
 Please apply!
 
 
 Mathias Krause (2):
   crypto: pcrypt - Fix wrong usage of rcu_dereference()
   padata: Fix wrong usage of rcu_dereference()
 

Both
Acked-by: Steffen Klassert steffen.klass...@secunet.com

Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crypto: skcipher - Use eseqiv even on UP machines

[Steffen Klassert]

On Thu, Oct 24, 2013 at 08:41:49PM +0800, Herbert Xu wrote:
 Hi:
 
 Previously we would use eseqiv on all async ciphers in all cases,
 and sync ciphers if we have more than one CPU.  This meant that
 chainiv is only used in the case of sync ciphers on a UP machine.
 
 As chainiv may aid attackers by making the IV predictable, even
 though this risk itself is small, the above usage pattern causes
 it to further leak information about the host.
 
 This patch addresses these issues by using eseqiv even if we're
 on a UP machine.
 
 Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
 

That's fine by me.

Acked-by: Steffen Klassert steffen.klass...@secunet.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] padata: make the sequence counter an atomic_t

[Steffen Klassert]

On Wed, Oct 02, 2013 at 03:40:45PM +0200, Mathias Krause wrote:
 Using a spinlock to atomically increase a counter sounds wrong -- we've
 atomic_t for this!
 
 Also move 'seq_nr' to a different cache line than 'lock' to reduce cache
 line trashing. This has the nice side effect of decreasing the size of
 struct parallel_data from 192 to 128 bytes for a x86-64 build, e.g.
 occupying only two instead of three cache lines.
 
 Those changes results in a 5% performance increase on an IPsec test run
 using pcrypt.
 
 Btw. the seq_lock spinlock was never explicitly initialized -- one more
 reason to get rid of it.
 
 Signed-off-by: Mathias Krause mathias.kra...@secunet.com

Acked-by: Steffen Klassert steffen.klass...@secunet.com

Herbert can you take this one?
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kernel/padata.c: Register hotcpu notifier after initialization

[Steffen Klassert]

On Fri, Aug 23, 2013 at 01:12:33PM +0200, Richard Weinberger wrote:
 padata_cpu_callback() takes pinst-lock, to avoid taking
 an uninitialized lock, register the notifier after it's
 initialization.
 
 Signed-off-by: Richard Weinberger rich...@nod.at

Looks ok,

Acked-by: Steffen Klassert steffen.klass...@secunet.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

[Steffen Klassert]

On Thu, Aug 22, 2013 at 02:43:37PM +0800, Chen Gang wrote:
 Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to
 CPU_DOWN_PREPARE and CPU_UP_CANCELED.
 
 It will fix 2 bugs:
 
   not check the return value of __padata_remove_cpu() and 
 __padata_add_cpu().
   need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED.
 
 
 Signed-off-by: Chen Gang gang.c...@asianux.com

This looks ok to me, Herbert can you take this one?

Acked-by: Steffen Klassert steffen.klass...@secunet.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kernel/padata.c: always check the return value of __padata_remove_cpu() and __padata_add_cpu()

[Steffen Klassert]

On Thu, Aug 22, 2013 at 01:27:16PM +0800, Chen Gang wrote:
 On 08/22/2013 01:11 PM, Steffen Klassert wrote:
  On Tue, Aug 20, 2013 at 11:44:31AM +0800, Chen Gang wrote:
 
  If this patch is correct, better to let CPU_ONLINE and CPU_DOWN_FAILED
  share the same code.
 
  And do we need a comment /* fall through */ between CPU_UP_CANCELED
  and CPU_DOWN_FAILED (or it is another bug, need a 'break' statement) ?
 
  At last, also better to let CPU_DOWN_PREPARE and CPU_UP_CANCELED share
  the same code (if need a 'break'), or share the most of code (if fall
  through).
 
  
  CPU_ONLINE and CPU_DOWN_FAILED can share the code. Same is true for
  CPU_DOWN_PREPARE and CPU_UP_CANCELED.
  
  Thanks!
  
  
 
 Thank you too.
 
 And need I send another patch for it ?
 
 Or just make by yourself (and better to mark me as Reported-by). :-)
 

You found the problem, feel free to send a patch.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] crypto: add CMAC support to CryptoAPI

[Steffen Klassert]

On Mon, Apr 08, 2013 at 10:48:44AM +0300, Jussi Kivilinna wrote:
 Patch adds support for NIST recommended block cipher mode CMAC to CryptoAPI.
 
 This work is based on Tom St Denis' earlier patch,
  http://marc.info/?l=linux-crypto-vgerm=135877306305466w=2
 
 Cc: Tom St Denis tstde...@elliptictech.com
 Signed-off-by: Jussi Kivilinna jussi.kivili...@iki.fi

This patch does not apply clean to the ipsec-next tree
because of some crypto changes I don't have in ipsec-next.
The IPsec part should apply to the cryptodev tree,
so it's probaply the best if we route this patchset
through the cryptodev tree.

Herbert,

are you going to take these patches?
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: authencesn compatibility problemn between software crypto and talitos driver

[Steffen Klassert]

Ccing Horia Geanta, he did the esn implementation for talitos.

On Fri, Mar 08, 2013 at 03:27:48PM +, Chaoxing Lin wrote:
 1. Can any one point me which RFC describe how exactly authencesn should work?
 

The ESN algorithm is described in RFC 4303 IP Encapsulating Security
Payload (ESP).

 2. I test Ipsec with esp=aes256-sha512-esn! options and found compatibility 
 issue between kernel software crypto and talitos driver.
 Talitos talitos Good
 Soft cryptosoft crypto  Good
 Soft cryptotalitos  link established but no traffic 
 can pass through.
 
 3. Looking at source code of latest stable kernel 3.8.2, I found that these 
 two implementations don't agree on what's to be hashed in ESN case.
 Talitos driver is more intuitive in that  assoc (SPI, SN-hi, SN-low) + IV + 
 payload are hashed.

The ESN implementation of the talitos driver looks rather scary,
it just renames authenc to authencesn in talitos_probe(). The
algorithm pretends to be authencesn but still does authenc, of course.
authencesn has to be implemented, it is not sufficient to change
the name, really.

 Kernel software crypto is counter-intuitive in that hsg(SPI, SN-low) + sg(IV 
 + payload) + tsg(SN-hi are hashed.

This might look counterintuitive, but that's what RFC 4303 describes
for ESN if separate encryption and integrity algorithms are used.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

On Wed, Jan 23, 2013 at 05:35:10PM +0200, Jussi Kivilinna wrote:
 
 Problem seems to be that PFKEYv2 does not quite work with IKEv2, and
 XFRM API should be used instead. There is new numbers assigned for
 IKEv2: 
 https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
 
 For new SADB_X_AALG_*, I'd think you should use value from Reserved
 for private use range. Maybe 250?

This would be an option, but we have just a few slots for private
algorithms.

 
 But maybe better solution might be to not make AES-CMAC (or other
 new algorithms) available throught PFKEY API at all, just XFRM?
 

It is probably the best to make new algorithms unavailable for pfkey
as long as they have no official ikev1 iana transform identifier.

But how to do that? Perhaps we can assign SADB_X_AALG_NOPFKEY to
the private value 255 and return -EINVAL if pfkey tries to register
such an algorithm. The netlink interface does not use these
identifiers, everything should work as expected. So it should be
possible to use these algoritms with iproute2 and the most modern
ike deamons.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
 
 Maybe it would be cleaner to not mess with pfkeyv2.h at all, but instead mark 
 algorithms that do not support pfkey with flag. See patch below.
 

Yes, would be an option too. I would be fine with that,
but let's here if someone else has an opinion on this.
Anyway, we need a solution to integrate Tom's patch soon.

 Then I started looking up if sadb_alg_id is being used somewhere outside 
 pfkey. Seems that its value is just being copied around.. but at 
 http://lxr.linux.no/linux+v3.7/net/xfrm/xfrm_policy.c#L1991; it's used as 
 bit-index. So do larger values than 31 break some stuff? Can multiple 
 algorithms have same sadb_alg_id value? Also in af_key.c, sadb_alg_id being 
 used as bit-index.
 

Herbert tried to address this already in git commit c5d18e984
([IPSEC]: Fix catch-22 with algorithm IDs above 31) some years
ago.

But this looks still messy. If the aalgos, ealgos and calgos mask is ~0, 
we allow all algorithms. If this is not the case, xfrm and pfkey check
the aalgos mask against the algorithm ID, only pfkey checks the ealgo
mask and noone checks the calgos mask.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

On Thu, Jan 24, 2013 at 07:37:50AM -0500, Tom St Denis wrote:
 
 Also a question for the netdev folk, in order to be timely would it be 
 acceptable to patch ah4 and then ah6 with the AEAD changes?  Or would the 
 team require both to be patched simultaneously?
  

We would need patches for both, but the changes to ah4/ah6 should
e very similar.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/9] kernel: padata : use __this_cpu_read per-cpu helper

[Steffen Klassert]

On Tue, Nov 13, 2012 at 09:52:33AM +0800, Shan Wei wrote:
 From: Shan Wei davids...@tencent.com
 
 For bottom halves off, __this_cpu_read is better.
 
 Signed-off-by: Shan Wei davids...@tencent.com
 Reviewed-by: Christoph Lameter c...@linux.com

Acked-by: Steffen Klassert steffen.klass...@secunet.com

Herbert, are you going to take this one?

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/9] kernel: padata : use this_cpu_read per-cpu helper

[Steffen Klassert]

  
 @@ -204,8 +204,7 @@ static struct padata_priv *padata_get_next(struct 
 parallel_data *pd)
   goto out;
   }
  
 - queue = per_cpu_ptr(pd-pqueue, smp_processor_id());
 - if (queue-cpu_index == next_queue-cpu_index) {
 + if (this_cpu_read(pd-pqueue-cpu_index) == next_queue-cpu_index) {

This runs with bottom halves off, so we can use __this_cpu_read here.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] netlink: add reference of module in netlink_dump_start

[Steffen Klassert]

On Wed, Sep 26, 2012 at 12:52:10PM +0800, Gao feng wrote:
 +
  int netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
  const struct nlmsghdr *nlh,
  struct netlink_dump_control *control)
 @@ -1786,6 +1794,7 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff 
 *skb,
   cb-done = control-done;
   cb-nlh = nlh;
   cb-data = control-data;
 + cb-module = control-module;
   cb-min_dump_alloc = control-min_dump_alloc;
   atomic_inc(skb-users);
   cb-skb = skb;
 @@ -1796,19 +1805,27 @@ int netlink_dump_start(struct sock *ssk, struct 
 sk_buff *skb,
   return -ECONNREFUSED;
   }
   nlk = nlk_sk(sk);
 - /* A dump is in progress... */
 +
   mutex_lock(nlk-cb_mutex);
 + /* A dump is in progress... */
   if (nlk-cb) {
   mutex_unlock(nlk-cb_mutex);
   netlink_destroy_callback(cb);
 - sock_put(sk);
 - return -EBUSY;
 + ret = -EBUSY;
 + goto out;
 + }
 + /* add reference of module witch cb-dump belone to */
 + if (cb-module  !try_module_get(cb-module)) {
 + mutex_unlock(nlk-cb_mutex);
 + ret = -EPROTONOSUPPORT;
 + goto out;

Looks like you leak the allocated netlink_callback here.
You should call netlink_destroy_callback() before you exit.

   }
 +
   nlk-cb = cb;
   mutex_unlock(nlk-cb_mutex);
  
   ret = netlink_dump(sk);
 -
 +out:
   sock_put(sk);
  
   if (ret)
 -- 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/11] crypto: pass crypto_user module to netlink_dump_start

[Steffen Klassert]

On Wed, Sep 26, 2012 at 12:52:17PM +0800, Gao feng wrote:
 use proper netlink_dump_control.done and .module to avoid panic.
 
 Signed-off-by: Gao feng gaof...@cn.fujitsu.com
 Cc: Herbert Xu herb...@gondor.apana.org.au
 ---
  crypto/crypto_user.c |6 +-
  1 files changed, 5 insertions(+), 1 deletions(-)
 
 diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
 index ba2c611..b5cb32b 100644
 --- a/crypto/crypto_user.c
 +++ b/crypto/crypto_user.c
 @@ -249,6 +249,7 @@ out_err:
  
  static int crypto_dump_report_done(struct netlink_callback *cb)
  {
 + netlink_dump_done(cb);
   return 0;

It's probaply better to return the return value of netlink_dump_done()
instead. Right now, netlink_dump_done() returns 0 in any case,
but this might change over time.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: iproute2 - IPsec ESN support

[Steffen Klassert]

On Wed, Jul 25, 2012 at 06:14:52AM +, Geanta Neag Horia Ioan-B05471 wrote:
 Hello,
 
 Is there any way to create an IPsec tunnel and indicate using
 extended sequnce numbers?

The strongswan ike deamon supports extended sequnce numbers.

 
 It seems that currently iproute2 doesn't support this.
 Grepping for esn reveals that XFRM_STATE_ESN shows only in kernel headers.
 
 The only relevant thing I found was a RFC sent by Steffen (Cc-ed),
 but it was never applied (don't know why):
 [RFC] iproute2: Add IPsec extended sequence number support

I'll take this as a reminder to respin this patch.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: algapi - Fix hang on crypto allocation

[Steffen Klassert]

git commit 398710379 (crypto: algapi - Move larval completion
into algboss) replaced accidentally a call to complete_all() by
a call to complete(). This causes a hang on crypto allocation
if we have more than one larval waiter. This pach restores the
call to complete_all().

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/algboss.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/crypto/algboss.c b/crypto/algboss.c
index f97027e..769219b 100644
--- a/crypto/algboss.c
+++ b/crypto/algboss.c
@@ -87,7 +87,7 @@ static int cryptomgr_probe(void *data)
crypto_tmpl_put(tmpl);
 
 out:
-   complete(param-completion);
+   complete_all(param-completion);
kfree(param);
module_put_and_exit(0);
 }
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Old PADATA patch vs crypto-2.6 tree

[Steffen Klassert]

On Wed, Mar 28, 2012 at 01:53:41PM +0200, Sebastien Agnolini wrote:
 Hey,
 
 How activate the IPsec parallelization ?

You need to instantiate pcrypt.

You need either crconf (linux-3.2 or newer with the crypto userconfig
api enabled) or the tcrypt module to instantiate pcrypt.

With crconf:

You can get crconf from https://sourceforge.net/projects/crconf/
Use the git tree for the newest version. After installing crconf
do e.g.

crconf add driver pcrypt(authenc(hmac(sha1-generic),cbc(aes-asm))) type 3


With tcrypt:

modprobe tcrypt alg=pcrypt(authenc(hmac(sha1-generic),cbc(aes-asm))) type=3

The modprobe will return with an error, don't worry about it, thats ok.



Once you've did one of the above, your /proc/crypto should show
something like:

name : authenc(hmac(sha1),cbc(aes))
driver   : pcrypt(authenc(hmac(sha1-generic),cbc(aes-asm)))
module   : pcrypt
priority : 2100
refcnt   : 1
selftest : passed
type : aead
async: yes
blocksize: 16
ivsize   : 16
maxauthsize  : 20
geniv: built-in


Now pcrypt is instantiated, e.g. all new IPsec states (that do
hmac-sha1, cbc-aes) will use it.


 I compiled the crypto-2.6 kernel with this param :
 CONFIG_CRYPTO_... = y
 CONFIG_PADATA = y
 CONFIG_SMP=y
 After installation on 2 servers (IPSEC tunnel), i don't detect the IPsec
 parallelization.
 The algorithm is loaded (present in /proc/crypto), but only one core works.
 
 So, What are the other parameters that I forgot for the compilation of the
 kernel? IRQ, IO, Scheduler parameters... Am i missing something ?
 I thought that the parallelization was automatically started. True ?

No, see above.

 What are the conditions to observe a parallel work ?

padata/pcrypt uses workqueues for parallelization, so you should see a
lot of busy kworkers when you put your IPsec network under load.
Also the refcnt (/proc/crypto) of the pcrypt algorithm should increase
when you add IPsec states.


--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] crypto: user - Fix size of netlink dump message

[Steffen Klassert]

On Thu, Mar 29, 2012 at 07:53:30AM +0200, Steffen Klassert wrote:
 
 Ok, so I'll respin this one to the mainline and resend.

I'll resend the whole patchset, please ignore this one.

Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] crypto: user - Fix lookup of algorithms with IV generator

[Steffen Klassert]

We lookup algorithms with crypto_alg_mod_lookup() when instantiating via
crypto_add_alg(). However, algorithms that are wrapped by an IV genearator
(e.g. aead or genicv type algorithms) need special care. The userspace
process hangs until it gets a timeout when we use crypto_alg_mod_lookup()
to lookup these algorithms. So export the lookup functions for these
algorithms and use them in crypto_add_alg().

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/ablkcipher.c|4 +-
 crypto/aead.c  |4 +-
 crypto/crypto_user.c   |   72 +++-
 include/crypto/internal/aead.h |2 +
 include/crypto/internal/skcipher.h |2 +
 5 files changed, 79 insertions(+), 5 deletions(-)

diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
index a0f768c..8d3a056 100644
--- a/crypto/ablkcipher.c
+++ b/crypto/ablkcipher.c
@@ -613,8 +613,7 @@ out:
return err;
 }
 
-static struct crypto_alg *crypto_lookup_skcipher(const char *name, u32 type,
-u32 mask)
+struct crypto_alg *crypto_lookup_skcipher(const char *name, u32 type, u32 mask)
 {
struct crypto_alg *alg;
 
@@ -652,6 +651,7 @@ static struct crypto_alg *crypto_lookup_skcipher(const char 
*name, u32 type,
 
return ERR_PTR(crypto_givcipher_default(alg, type, mask));
 }
+EXPORT_SYMBOL_GPL(crypto_lookup_skcipher);
 
 int crypto_grab_skcipher(struct crypto_skcipher_spawn *spawn, const char *name,
 u32 type, u32 mask)
diff --git a/crypto/aead.c b/crypto/aead.c
index 04add3d..e4cb351 100644
--- a/crypto/aead.c
+++ b/crypto/aead.c
@@ -470,8 +470,7 @@ out:
return err;
 }
 
-static struct crypto_alg *crypto_lookup_aead(const char *name, u32 type,
-u32 mask)
+struct crypto_alg *crypto_lookup_aead(const char *name, u32 type, u32 mask)
 {
struct crypto_alg *alg;
 
@@ -503,6 +502,7 @@ static struct crypto_alg *crypto_lookup_aead(const char 
*name, u32 type,
 
return ERR_PTR(crypto_nivaead_default(alg, type, mask));
 }
+EXPORT_SYMBOL_GPL(crypto_lookup_aead);
 
 int crypto_grab_aead(struct crypto_aead_spawn *spawn, const char *name,
 u32 type, u32 mask)
diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index f76e42b..e91c161 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -21,9 +21,13 @@
 #include linux/module.h
 #include linux/crypto.h
 #include linux/cryptouser.h
+#include linux/sched.h
 #include net/netlink.h
 #include linux/security.h
 #include net/net_namespace.h
+#include crypto/internal/aead.h
+#include crypto/internal/skcipher.h
+
 #include internal.h
 
 DEFINE_MUTEX(crypto_cfg_mutex);
@@ -301,6 +305,60 @@ static int crypto_del_alg(struct sk_buff *skb, struct 
nlmsghdr *nlh,
return crypto_unregister_instance(alg);
 }
 
+static struct crypto_alg *crypto_user_skcipher_alg(const char *name, u32 type,
+  u32 mask)
+{
+   int err;
+   struct crypto_alg *alg;
+
+   type = crypto_skcipher_type(type);
+   mask = crypto_skcipher_mask(mask);
+
+   for (;;) {
+   alg = crypto_lookup_skcipher(name,  type, mask);
+   if (!IS_ERR(alg))
+   return alg;
+
+   err = PTR_ERR(alg);
+   if (err != -EAGAIN)
+   break;
+   if (signal_pending(current)) {
+   err = -EINTR;
+   break;
+   }
+   }
+
+   return ERR_PTR(err);
+}
+
+static struct crypto_alg *crypto_user_aead_alg(const char *name, u32 type,
+  u32 mask)
+{
+   int err;
+   struct crypto_alg *alg;
+
+   type = ~(CRYPTO_ALG_TYPE_MASK | CRYPTO_ALG_GENIV);
+   type |= CRYPTO_ALG_TYPE_AEAD;
+   mask = ~(CRYPTO_ALG_TYPE_MASK | CRYPTO_ALG_GENIV);
+   mask |= CRYPTO_ALG_TYPE_MASK;
+
+   for (;;) {
+   alg = crypto_lookup_aead(name,  type, mask);
+   if (!IS_ERR(alg))
+   return alg;
+
+   err = PTR_ERR(alg);
+   if (err != -EAGAIN)
+   break;
+   if (signal_pending(current)) {
+   err = -EINTR;
+   break;
+   }
+   }
+
+   return ERR_PTR(err);
+}
+
 static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
  struct nlattr **attrs)
 {
@@ -325,7 +383,19 @@ static int crypto_add_alg(struct sk_buff *skb, struct 
nlmsghdr *nlh,
else
name = p-cru_name;
 
-   alg = crypto_alg_mod_lookup(name, p-cru_type, p-cru_mask);
+   switch (p-cru_type  p-cru_mask  CRYPTO_ALG_TYPE_MASK) {
+   case CRYPTO_ALG_TYPE_AEAD:
+   alg = crypto_user_aead_alg(name, p-cru_type, p-cru_mask);
+   break;
+   case

[PATCH v2 2/2] crypto: user - Fix size of netlink dump message

[Steffen Klassert]

The default netlink message size limit might be exceeded when dumping a
lot of algorithms to userspace. As a result, not all of the instantiated
algorithms dumped to userspace. So calculate an upper bound on the message
size and call netlink_dump_start() with that value.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/crypto_user.c   |8 
 include/linux/cryptouser.h |3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index e91c161..f1ea0a0 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -457,12 +457,20 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, 
struct nlmsghdr *nlh)
 
if ((type == (CRYPTO_MSG_GETALG - CRYPTO_MSG_BASE) 
(nlh-nlmsg_flags  NLM_F_DUMP))) {
+   struct crypto_alg *alg;
+   u16 dump_alloc = 0;
+
if (link-dump == NULL)
return -EINVAL;
+
+   list_for_each_entry(alg, crypto_alg_list, cra_list)
+   dump_alloc += CRYPTO_REPORT_MAXSIZE;
+
{
struct netlink_dump_control c = {
.dump = link-dump,
.done = link-done,
+   .min_dump_alloc = dump_alloc,
};
return netlink_dump_start(crypto_nlsk, skb, nlh, c);
}
diff --git a/include/linux/cryptouser.h b/include/linux/cryptouser.h
index 532fb58..4abf2ea 100644
--- a/include/linux/cryptouser.h
+++ b/include/linux/cryptouser.h
@@ -100,3 +100,6 @@ struct crypto_report_rng {
char type[CRYPTO_MAX_NAME];
unsigned int seedsize;
 };
+
+#define CRYPTO_REPORT_MAXSIZE (sizeof(struct crypto_user_alg) + \
+  sizeof(struct crypto_report_blkcipher))
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

padata: Fixes for 3.4

[Steffen Klassert]

This patchset contains the following changes:

1) Add a reference to the padata api documentation to the code.
   Suggested by Peter Zijlstra.

2) We use the active cpumask to determine the superset of cpus
   to use for parallelization. The active cpumask is not the appropriate
   cpumask for these purposes. Replace the cpu active usage by cpu online.
   Reported by Peter Zijlstra.

3)  On cpu hotplug, we don't remove the cpu that went offline from our cpumasks.
Fix this by removing this cpu from the padata cpumasks.

Please pull or apply.

The patchset is based on the crypto-2.6 tree and is available via git:

The following changes since commit ff0a70fe053614e763eb3ac88bfea9c5615fce3b:
  Jussi Kivilinna (1):
crypto: twofish-x86_64-3way - module init/exit functions should be 
static

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-stk padata-fixes

Steffen Klassert (3):
  padata: Add a reference to the api documentation
  padata: Use the online cpumask as the default
  padata: Fix cpu hotplug

 kernel/padata.c |   13 +
 1 files changed, 9 insertions(+), 4 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] padata: Add a reference to the api documentation

[Steffen Klassert]

Add a reference to the padata api documentation at Documentation/padata.txt

Suggested-by: Peter Zijlstra pet...@infradead.org
Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 kernel/padata.c |2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index 6f10eb2..7875088 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -1,6 +1,8 @@
 /*
  * padata.c - generic interface to process data streams in parallel
  *
+ * See Documentation/padata.txt for an api documentation.
+ *
  * Copyright (C) 2008, 2009 secunet Security Networks AG
  * Copyright (C) 2008, 2009 Steffen Klassert steffen.klass...@secunet.com
  *
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] padata: Fix cpu hotplug

[Steffen Klassert]

We don't remove the cpu that went offline from our cpumasks
on cpu hotplug. This got lost somewhere along the line, so
restore it. This fixes a hang of the padata instance on cpu
hotplug.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 kernel/padata.c |3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index de3d0d9..89fe3d1 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -748,6 +748,9 @@ static int __padata_remove_cpu(struct padata_instance 
*pinst, int cpu)
return -ENOMEM;
 
padata_replace(pinst, pd);
+
+   cpumask_clear_cpu(cpu, pd-cpumask.cbcpu);
+   cpumask_clear_cpu(cpu, pd-cpumask.pcpu);
}
 
return 0;
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] padata: Use the online cpumask as the default

[Steffen Klassert]

We use the active cpumask to determine the superset of cpus
to use for parallelization. However, the active cpumask is
for internal usage of the scheduler and therefore not the
appropriate cpumask for these purposes. So use the online
cpumask instead.

Reported-by: Peter Zijlstra pet...@infradead.org
Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 kernel/padata.c |8 
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/padata.c b/kernel/padata.c
index 7875088..de3d0d9 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -356,13 +356,13 @@ static int padata_setup_cpumasks(struct parallel_data *pd,
if (!alloc_cpumask_var(pd-cpumask.pcpu, GFP_KERNEL))
return -ENOMEM;
 
-   cpumask_and(pd-cpumask.pcpu, pcpumask, cpu_active_mask);
+   cpumask_and(pd-cpumask.pcpu, pcpumask, cpu_online_mask);
if (!alloc_cpumask_var(pd-cpumask.cbcpu, GFP_KERNEL)) {
free_cpumask_var(pd-cpumask.cbcpu);
return -ENOMEM;
}
 
-   cpumask_and(pd-cpumask.cbcpu, cbcpumask, cpu_active_mask);
+   cpumask_and(pd-cpumask.cbcpu, cbcpumask, cpu_online_mask);
return 0;
 }
 
@@ -566,7 +566,7 @@ EXPORT_SYMBOL(padata_unregister_cpumask_notifier);
 static bool padata_validate_cpumask(struct padata_instance *pinst,
const struct cpumask *cpumask)
 {
-   if (!cpumask_intersects(cpumask, cpu_active_mask)) {
+   if (!cpumask_intersects(cpumask, cpu_online_mask)) {
pinst-flags |= PADATA_INVALID;
return false;
}
@@ -680,7 +680,7 @@ static int __padata_add_cpu(struct padata_instance *pinst, 
int cpu)
 {
struct parallel_data *pd;
 
-   if (cpumask_test_cpu(cpu, cpu_active_mask)) {
+   if (cpumask_test_cpu(cpu, cpu_online_mask)) {
pd = padata_alloc_pd(pinst, pinst-cpumask.pcpu,
 pinst-cpumask.cbcpu);
if (!pd)
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: pcrypt - Use the online cpumask as the default

[Steffen Klassert]

We use the active cpumask to determine the superset of cpus
to use for parallelization. However, the active cpumask is
for internal usage of the scheduler and therefore not the
appropriate cpumask for these purposes. So use the online
cpumask instead.

Reported-by: Peter Zijlstra pet...@infradead.org
Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/pcrypt.c |8 
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index 29a89da..b2c99dc 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -280,11 +280,11 @@ static int pcrypt_aead_init_tfm(struct crypto_tfm *tfm)
 
ictx-tfm_count++;
 
-   cpu_index = ictx-tfm_count % cpumask_weight(cpu_active_mask);
+   cpu_index = ictx-tfm_count % cpumask_weight(cpu_online_mask);
 
-   ctx-cb_cpu = cpumask_first(cpu_active_mask);
+   ctx-cb_cpu = cpumask_first(cpu_online_mask);
for (cpu = 0; cpu  cpu_index; cpu++)
-   ctx-cb_cpu = cpumask_next(ctx-cb_cpu, cpu_active_mask);
+   ctx-cb_cpu = cpumask_next(ctx-cb_cpu, cpu_online_mask);
 
cipher = crypto_spawn_aead(crypto_instance_ctx(inst));
 
@@ -472,7 +472,7 @@ static int pcrypt_init_padata(struct padata_pcrypt *pcrypt,
goto err_free_padata;
}
 
-   cpumask_and(mask-mask, cpu_possible_mask, cpu_active_mask);
+   cpumask_and(mask-mask, cpu_possible_mask, cpu_online_mask);
rcu_assign_pointer(pcrypt-cb_cpumask, mask);
 
pcrypt-nblock.notifier_call = pcrypt_cpumask_change_notify;
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] crypto: user - Fix size of netlink dump message

[Steffen Klassert]

On Mon, Mar 26, 2012 at 09:28:27AM +0200, Steffen Klassert wrote:
 +
   return netlink_dump_start(crypto_nlsk, skb, nlh,
 -   link-dump, link-done, 0);
 +   link-dump, link-done, dump_alloc);

I've just noticed that the interface to netlink_dump_start() changed in
the mainline (with commit 80d326fab534a5380e8f6e509a0b9076655a9670), so
this will cause a conflict. The crypto-2.6 tree is way behind the mainline
(v3.2 + some patches). Could you please update the crypto-2.6 tree so that
I can respin this patch?

Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] crypto: user - Fix size of netlink dump message

[Steffen Klassert]

On Thu, Mar 29, 2012 at 01:46:26PM +0800, Herbert Xu wrote:
 On Thu, Mar 29, 2012 at 07:45:04AM +0200, Steffen Klassert wrote:
  On Mon, Mar 26, 2012 at 09:28:27AM +0200, Steffen Klassert wrote:
   +
 return netlink_dump_start(crypto_nlsk, skb, nlh,
   -   link-dump, link-done, 0);
   +   link-dump, link-done, dump_alloc);
  
  I've just noticed that the interface to netlink_dump_start() changed in
  the mainline (with commit 80d326fab534a5380e8f6e509a0b9076655a9670), so
  this will cause a conflict. The crypto-2.6 tree is way behind the mainline
  (v3.2 + some patches). Could you please update the crypto-2.6 tree so that
  I can respin this patch?
 
 
 As long as your patch is against mainline it should be fine.  I will
 rebase cryptodev before applying any patches for the next release.
 

Ok, so I'll respin this one to the mainline and resend.
Anyway, would be nice to see this fix in v3.4.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] crypto: user - Fix lookup of algorithms with IV generator

[Steffen Klassert]

We lookup algorithms with crypto_alg_mod_lookup() when instantiating via
crypto_add_alg(). However, algorithms that are wrapped by an IV genearator
(e.g. aead or givcipher type algorithms) need special care. The userspace
process hangs until it gets a timeout when we use crypto_alg_mod_lookup()
to lookup these algorithms. So export the lookup functions for these
algorithms and use them when instantiating via crypto_add_alg().

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/ablkcipher.c|4 +-
 crypto/aead.c  |4 +-
 crypto/crypto_user.c   |   71 +++-
 include/crypto/internal/aead.h |2 +
 include/crypto/internal/skcipher.h |2 +
 5 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/crypto/ablkcipher.c b/crypto/ablkcipher.c
index a0f768c..8d3a056 100644
--- a/crypto/ablkcipher.c
+++ b/crypto/ablkcipher.c
@@ -613,8 +613,7 @@ out:
return err;
 }
 
-static struct crypto_alg *crypto_lookup_skcipher(const char *name, u32 type,
-u32 mask)
+struct crypto_alg *crypto_lookup_skcipher(const char *name, u32 type, u32 mask)
 {
struct crypto_alg *alg;
 
@@ -652,6 +651,7 @@ static struct crypto_alg *crypto_lookup_skcipher(const char 
*name, u32 type,
 
return ERR_PTR(crypto_givcipher_default(alg, type, mask));
 }
+EXPORT_SYMBOL_GPL(crypto_lookup_skcipher);
 
 int crypto_grab_skcipher(struct crypto_skcipher_spawn *spawn, const char *name,
 u32 type, u32 mask)
diff --git a/crypto/aead.c b/crypto/aead.c
index 04add3d..e4cb351 100644
--- a/crypto/aead.c
+++ b/crypto/aead.c
@@ -470,8 +470,7 @@ out:
return err;
 }
 
-static struct crypto_alg *crypto_lookup_aead(const char *name, u32 type,
-u32 mask)
+struct crypto_alg *crypto_lookup_aead(const char *name, u32 type, u32 mask)
 {
struct crypto_alg *alg;
 
@@ -503,6 +502,7 @@ static struct crypto_alg *crypto_lookup_aead(const char 
*name, u32 type,
 
return ERR_PTR(crypto_nivaead_default(alg, type, mask));
 }
+EXPORT_SYMBOL_GPL(crypto_lookup_aead);
 
 int crypto_grab_aead(struct crypto_aead_spawn *spawn, const char *name,
 u32 type, u32 mask)
diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index 3e61cc1..38534aa 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -24,6 +24,9 @@
 #include net/netlink.h
 #include linux/security.h
 #include net/net_namespace.h
+#include crypto/internal/aead.h
+#include crypto/internal/skcipher.h
+
 #include internal.h
 
 DEFINE_MUTEX(crypto_cfg_mutex);
@@ -301,6 +304,60 @@ static int crypto_del_alg(struct sk_buff *skb, struct 
nlmsghdr *nlh,
return crypto_unregister_instance(alg);
 }
 
+static struct crypto_alg *crypto_user_skcipher_alg(const char *name, u32 type,
+  u32 mask)
+{
+   int err;
+   struct crypto_alg *alg;
+
+   type = crypto_skcipher_type(type);
+   mask = crypto_skcipher_mask(mask);
+
+   for (;;) {
+   alg = crypto_lookup_skcipher(name,  type, mask);
+   if (!IS_ERR(alg))
+   return alg;
+
+   err = PTR_ERR(alg);
+   if (err != -EAGAIN)
+   break;
+   if (signal_pending(current)) {
+   err = -EINTR;
+   break;
+   }
+   }
+
+   return ERR_PTR(err);
+}
+
+static struct crypto_alg *crypto_user_aead_alg(const char *name, u32 type,
+  u32 mask)
+{
+   int err;
+   struct crypto_alg *alg;
+
+   type = ~(CRYPTO_ALG_TYPE_MASK | CRYPTO_ALG_GENIV);
+   type |= CRYPTO_ALG_TYPE_AEAD;
+   mask = ~(CRYPTO_ALG_TYPE_MASK | CRYPTO_ALG_GENIV);
+   mask |= CRYPTO_ALG_TYPE_MASK;
+
+   for (;;) {
+   alg = crypto_lookup_aead(name,  type, mask);
+   if (!IS_ERR(alg))
+   return alg;
+
+   err = PTR_ERR(alg);
+   if (err != -EAGAIN)
+   break;
+   if (signal_pending(current)) {
+   err = -EINTR;
+   break;
+   }
+   }
+
+   return ERR_PTR(err);
+}
+
 static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
  struct nlattr **attrs)
 {
@@ -325,7 +382,19 @@ static int crypto_add_alg(struct sk_buff *skb, struct 
nlmsghdr *nlh,
else
name = p-cru_name;
 
-   alg = crypto_alg_mod_lookup(name, p-cru_type, p-cru_mask);
+   switch (p-cru_type  p-cru_mask  CRYPTO_ALG_TYPE_MASK) {
+   case CRYPTO_ALG_TYPE_AEAD:
+   alg = crypto_user_aead_alg(name, p-cru_type, p-cru_mask);
+   break;
+   case CRYPTO_ALG_TYPE_GIVCIPHER:
+   case CRYPTO_ALG_TYPE_BLKCIPHER:
+   case

[PATCH 2/2] crypto: user - Fix size of netlink dump message

[Steffen Klassert]

The default netlink message size limit might be exceeded when dumping a
lot of algorithms to userspace. As a result, not all of the instantiated
algorithms dumped to userspace. So calculate an upper bound on the message
size and call netlink_dump_start() with that value.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/crypto_user.c   |8 +++-
 include/linux/cryptouser.h |3 +++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index 38534aa..7fca04c 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -456,11 +456,17 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, 
struct nlmsghdr *nlh)
 
if ((type == (CRYPTO_MSG_GETALG - CRYPTO_MSG_BASE) 
(nlh-nlmsg_flags  NLM_F_DUMP))) {
+   struct crypto_alg *alg;
+   u16 dump_alloc = 0;
+
if (link-dump == NULL)
return -EINVAL;
 
+   list_for_each_entry(alg, crypto_alg_list, cra_list)
+   dump_alloc += CRYPTO_REPORT_MAXSIZE;
+
return netlink_dump_start(crypto_nlsk, skb, nlh,
- link-dump, link-done, 0);
+ link-dump, link-done, dump_alloc);
}
 
err = nlmsg_parse(nlh, crypto_msg_min[type], attrs, CRYPTOCFGA_MAX,
diff --git a/include/linux/cryptouser.h b/include/linux/cryptouser.h
index 532fb58..4abf2ea 100644
--- a/include/linux/cryptouser.h
+++ b/include/linux/cryptouser.h
@@ -100,3 +100,6 @@ struct crypto_report_rng {
char type[CRYPTO_MAX_NAME];
unsigned int seedsize;
 };
+
+#define CRYPTO_REPORT_MAXSIZE (sizeof(struct crypto_user_alg) + \
+  sizeof(struct crypto_report_blkcipher))
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] padata: Fix race on sequence number wrap

[Steffen Klassert]

When padata_do_parallel() is called from multiple cpus for the same
padata instance, we can get object reordering on sequence number wrap
because testing for sequence number wrap and reseting the sequence
number must happen atomically but is implemented with two atomic
operations. This patch fixes this by converting the sequence number
from atomic_t to an unsigned int and protect the access with a
spin_lock. As a side effect, we get rid of the sequence number wrap
handling because the seqence number wraps back to null now without
the need to do anything.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 include/linux/padata.h |6 ++
 kernel/padata.c|   38 ++
 2 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/include/linux/padata.h b/include/linux/padata.h
index 4633b2f..86292be 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -46,7 +46,6 @@ struct padata_priv {
struct list_headlist;
struct parallel_data*pd;
int cb_cpu;
-   int seq_nr;
int info;
void(*parallel)(struct padata_priv *padata);
void(*serial)(struct padata_priv *padata);
@@ -116,7 +115,6 @@ struct padata_cpumask {
  * @pinst: padata instance.
  * @pqueue: percpu padata queues used for parallelization.
  * @squeue: percpu padata queues used for serialuzation.
- * @seq_nr: The sequence number that will be attached to the next object.
  * @reorder_objects: Number of objects waiting in the reorder queues.
  * @refcnt: Number of objects holding a reference on this parallel_data.
  * @max_seq_nr:  Maximal used sequence number.
@@ -129,12 +127,12 @@ struct parallel_data {
struct padata_instance  *pinst;
struct padata_parallel_queue__percpu *pqueue;
struct padata_serial_queue  __percpu *squeue;
-   atomic_tseq_nr;
atomic_treorder_objects;
atomic_trefcnt;
-   unsigned intmax_seq_nr;
struct padata_cpumask   cpumask;
spinlock_t  lock cacheline_aligned;
+   spinlock_t  seq_lock;
+   unsigned intseq_nr;
unsigned intprocessed;
struct timer_list   timer;
 };
diff --git a/kernel/padata.c b/kernel/padata.c
index aa99295..6f10eb2 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -29,7 +29,6 @@
 #include linux/sysfs.h
 #include linux/rcupdate.h
 
-#define MAX_SEQ_NR (INT_MAX - NR_CPUS)
 #define MAX_OBJ_NUM 1000
 
 static int padata_index_to_cpu(struct parallel_data *pd, int cpu_index)
@@ -43,18 +42,19 @@ static int padata_index_to_cpu(struct parallel_data *pd, 
int cpu_index)
return target_cpu;
 }
 
-static int padata_cpu_hash(struct padata_priv *padata)
+static int padata_cpu_hash(struct parallel_data *pd)
 {
int cpu_index;
-   struct parallel_data *pd;
-
-   pd =  padata-pd;
 
/*
 * Hash the sequence numbers to the cpus by taking
 * seq_nr mod. number of cpus in use.
 */
-   cpu_index =  padata-seq_nr % cpumask_weight(pd-cpumask.pcpu);
+
+   spin_lock(pd-seq_lock);
+   cpu_index =  pd-seq_nr % cpumask_weight(pd-cpumask.pcpu);
+   pd-seq_nr++;
+   spin_unlock(pd-seq_lock);
 
return padata_index_to_cpu(pd, cpu_index);
 }
@@ -132,12 +132,7 @@ int padata_do_parallel(struct padata_instance *pinst,
padata-pd = pd;
padata-cb_cpu = cb_cpu;
 
-   if (unlikely(atomic_read(pd-seq_nr) == pd-max_seq_nr))
-   atomic_set(pd-seq_nr, -1);
-
-   padata-seq_nr = atomic_inc_return(pd-seq_nr);
-
-   target_cpu = padata_cpu_hash(padata);
+   target_cpu = padata_cpu_hash(pd);
queue = per_cpu_ptr(pd-pqueue, target_cpu);
 
spin_lock(queue-parallel.lock);
@@ -173,7 +168,7 @@ EXPORT_SYMBOL(padata_do_parallel);
 static struct padata_priv *padata_get_next(struct parallel_data *pd)
 {
int cpu, num_cpus;
-   int next_nr, next_index;
+   unsigned int next_nr, next_index;
struct padata_parallel_queue *queue, *next_queue;
struct padata_priv *padata;
struct padata_list *reorder;
@@ -189,14 +184,6 @@ static struct padata_priv *padata_get_next(struct 
parallel_data *pd)
cpu = padata_index_to_cpu(pd, next_index);
next_queue = per_cpu_ptr(pd-pqueue, cpu);
 
-   if (unlikely(next_nr  pd-max_seq_nr)) {
-   next_nr = next_nr - pd-max_seq_nr - 1;
-   next_index = next_nr % num_cpus;
-   cpu = padata_index_to_cpu(pd, next_index);
-   next_queue = per_cpu_ptr(pd-pqueue, cpu);
-   pd-processed = 0;
-   }
-
padata = NULL;
 
reorder = next_queue-reorder;
@@ -205,8 +192,6

Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0

[Steffen Klassert]

On Thu, Feb 02, 2012 at 03:42:43PM +0100, Jesper Juhl wrote:
 On Thu, 2 Feb 2012, Steffen Klassert wrote:
  
  Your first patch is correct.
  
 Thank you for the explanation.
 
 Can I take that to mean that I can add your Acked-by: if/when I resend the 
 patch?
 

Sure,

Acked-by: Steffen Klassert steffen.klass...@secunet.com
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] In crypto_add_alg(), 'exact' wants to be initialized to 0

[Steffen Klassert]

On Wed, Feb 01, 2012 at 09:21:39PM +0100, Jesper Juhl wrote:
 On Wed, 1 Feb 2012, devendra.aaru wrote:
 
  On Sun, Jan 29, 2012 at 5:39 PM, Jesper Juhl j...@chaosbits.net wrote:
   We declare 'exact' without initializing it and then do:
  
   [...]
          if (strlen(p-cru_driver_name))
                  exact = 1;
  
          if (priority  !exact)
                  return -EINVAL;
  
   [...]
  
   If the first 'if' is not true, then the second will test an
   uninitialized 'exact'.
  
  not needed . as the cru_driver_name will always be present :).
 
 If that is indeed the case, and we are guaranteed that, then it would seem 
 that a patch like the following would be what we want instead??
 
 Please note that this patch is intended just for discussion, nothing else 
 (which is why I left out a Signed-off-by on purpose), since I've not 
 tested it beyond checking that it compiles, nor have I verified your claim 
 that cru_driver_name will always be present.
 

We get cru_driver_name from a netlink message that a user sends us.
So it depends pretty much on the user whether cru_driver_name is
set or not. Usually it is set when a user wants to instantiate
a certain algorithm driver, like cbc(aes-asm). If the user just
wants to instantiate the system default of an algorithm, he can
set cru_name (e.g. to cbc(aes)) instead of cru_driver_name.

Your first patch is correct.

Thanks,

Steffen

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sha512: make it work, undo percpu message schedule

[Steffen Klassert]

On Fri, Jan 13, 2012 at 11:35:42AM +0100, Eric Dumazet wrote:
 Le vendredi 13 janvier 2012 à 18:08 +1100, Herbert Xu a écrit :
  On Fri, Jan 13, 2012 at 02:55:14AM +0300, Alexey Dobriyan wrote:
  
   Herbert, I couldn't come up with a single scenario. :-(
   But the bug is easy to reproduce.
  
  OK, does this patch work for you?
  
  commit 31f4e55c09c1170f8b813c14b1299b70f50db414
  Author: Herbert Xu herb...@gondor.apana.org.au
  Date:   Fri Jan 13 18:06:50 2012 +1100
  
  crypto: sha512 - Fix msg_schedule race
  
  The percpu msg_schedule setup was unsafe as a user in a process
  context can be interrupted by a softirq user which would then
  scribble over the exact same work area.  This was discovered by
  Steffen Klassert.
  
  This patch based on ideas from Eric Dumazet fixes this by using
  two independent work areas.
  
  Reported-by: Alexey Dobriyan adobri...@gmail.com
  Signed-off-by: Herbert Xu herb...@gondor.apana.org.au
  
 
 I wonder ...
 
 With 4096 cpus, do we really want to reserve 5242880 bytes of memory for
 this function ?
 
 What about following patch instead ?
 
 (Trying a dynamic memory allocation, and fallback on a single
 pre-allocated bloc of memory, shared by all cpus, protected by a
 spinlock)

If we want to do dynamic memory allocation, we could place it to the
shash_desc context where we already store the intermediate digest value.
This is preallocated anyway, so we don't need to do another allocation.

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sha512: make it work, undo percpu message schedule

[Steffen Klassert]

On Wed, Jan 11, 2012 at 11:36:11AM +1100, Herbert Xu wrote:
 On Wed, Jan 11, 2012 at 03:00:40AM +0300, Alexey Dobriyan wrote:
  commit f9e2bca6c22d75a289a349f869701214d63b5060
  aka crypto: sha512 - Move message schedule W[80] to static percpu area
  created global message schedule area.
  
  If sha512_update will ever be entered twice, hilarity ensures.
 
 Hmm, do you know why this happens? On the face of it this shouldn't
 be possible as preemption is disabled.
 

I did not try to reproduce, but this looks like a race of the 'local out'
and the receive packet path. On 'lokal out' bottom halves are enabled,
so could be interrupted by the NET_RX_SOFTIRQ while doing a sha512_update.
The NET_RX_SOFTIRQ could invoke sha512_update too, that would corrupt the
hash value. My guess could be checked easily by disabling the bottom halves
before the percpu value is fetched.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: crypto_ahash_setkey

[Steffen Klassert]

Hi.

On Wed, Nov 23, 2011 at 12:00:29AM +0200, Kasatkin, Dmitry wrote:
 Hi,
 
 I have noticed very odd behavior with hmac calculation on my dual
 core, 4 HTs PC.
 I am using async hash API to to calculate hmac over the page.
 I am using hmac(sha1) and the same key to calculate different pages.
 
 I have a work queue, which calculates the hmac like...
 
 int()
 {
 tfm = crypto_alloc_ahash(...);
 }
 
 work_task()
 {
  crypto_ahash_setkey(tfm, key, keylen);
  crypto_ahash_digest(req);
 }
 
 HMAC result sometimes is incorrect.

Looks like a race. HMAC precalculates the hash of the ipaded/opaded key
and saves this hash on the transform. So the setkey method should be used
just once in the initialization path.

 
 But when I move crypto_ahash_setkey() do the initialization code then
 HMAC result is always correct...
 (key is the same, so I can initialize it only once)
 
 int()
 {
  tfm = crypto_alloc_ahash(...);
  crypto_ahash_setkey(tfm, key, keylen);
 }

That's how it should be. And in this case it works, as you
already noticed :)

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] crypto: Unlink and free instances when deleted

[Steffen Klassert]

We leak the crypto instance when we unregister an instance with
crypto_del_alg(). Therefore we introduce crypto_unregister_instance()
to unlink the crypto instance from the template's instances list and
to free the recources of the instance properly.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/algapi.c |   29 +
 crypto/crypto_user.c|2 +-
 include/crypto/algapi.h |1 +
 3 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 54dd4e3..9d4a9fe 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -518,6 +518,35 @@ err:
 }
 EXPORT_SYMBOL_GPL(crypto_register_instance);
 
+int crypto_unregister_instance(struct crypto_alg *alg)
+{
+   int err;
+   struct crypto_instance *inst = (void *)alg;
+   struct crypto_template *tmpl = inst-tmpl;
+   LIST_HEAD(users);
+
+   if (!(alg-cra_flags  CRYPTO_ALG_INSTANCE))
+   return -EINVAL;
+
+   BUG_ON(atomic_read(alg-cra_refcnt) != 1);
+
+   down_write(crypto_alg_sem);
+
+   hlist_del_init(inst-list);
+   err = crypto_remove_alg(alg, users);
+
+   up_write(crypto_alg_sem);
+
+   if (err)
+   return err;
+
+   tmpl-free(inst);
+   crypto_remove_final(users);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(crypto_unregister_instance);
+
 int crypto_init_spawn(struct crypto_spawn *spawn, struct crypto_alg *alg,
  struct crypto_instance *inst, u32 mask)
 {
diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c
index 0605a2b..3ba6ef5 100644
--- a/crypto/crypto_user.c
+++ b/crypto/crypto_user.c
@@ -298,7 +298,7 @@ static int crypto_del_alg(struct sk_buff *skb, struct 
nlmsghdr *nlh,
if (atomic_read(alg-cra_refcnt) != 1)
return -EBUSY;
 
-   return crypto_unregister_alg(alg);
+   return crypto_unregister_instance(alg);
 }
 
 static int crypto_add_alg(struct sk_buff *skb, struct nlmsghdr *nlh,
diff --git a/include/crypto/algapi.h b/include/crypto/algapi.h
index ecc721d..418d270 100644
--- a/include/crypto/algapi.h
+++ b/include/crypto/algapi.h
@@ -134,6 +134,7 @@ struct crypto_template *crypto_lookup_template(const char 
*name);
 
 int crypto_register_instance(struct crypto_template *tmpl,
 struct crypto_instance *inst);
+int crypto_unregister_instance(struct crypto_alg *alg);
 
 int crypto_init_spawn(struct crypto_spawn *spawn, struct crypto_alg *alg,
  struct crypto_instance *inst, u32 mask);
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: CONFIG_CRYPTO_PCRYPT, CONFIG_CRYPTO_CRYPTD etc. questions

[Steffen Klassert]

On Sun, Oct 09, 2011 at 01:00:08AM +0200, Sverd Johnsen wrote:
 Hey,
 
 how do they work? Should one use them? Does it work with all ciphers?
 Do they exclude each other?

They don't exclude each other, but using both in an algorithm instance
does not make much sense. It depends on your usecase what to choose.

If a single cpu sends a lot of crypto requests (like it happens on
IPsec networking), it makes sense to parallelize with pcrypt. 

Usually you don't need to setup a cryptd algorithm expicit. If an
algorithm needs cryptd, it cooses it automatically. It is used e.g.
to move the crypto requests out of atomic context (AES-NI does this.)

 
 Do they need module parameters? What if they are built-in?

They don't need module parameters. It does not matter much if they
are build-in or a module. If they are build-in, they can be just
instantiated. If they are build as module, the crypto layer tries
to autoload the module if it is not already loaded.

If you want to use pcrypt/cryptd on an algorithm that does not coose
them automatically, you have to instantiate (load) these algorithms.

Right now, using 'tcrypt' is the only way to instantiate such nonstandard
algorithms. That's kind of a hack, but things are getting much easier
as soon as we have the crypto userspace configuration API merged.

 How can I verify that they work?

You can look at /proc/crypto, this shows which algorithms are used.
You should find cryptd/pcrypt in the algorithm drivers name if they
are instantiated.

 How do they work? There is absolutely no fucking information
 whatsoever, neither in the help text nor in the docs. Am i missing
 something?

Indeed, there is no documentation.

 
 Talking mostly about typical disk encryption on SMP machines via
 dm-crypt, interested in best performance. (without AES-NI)

dm-crypt uses it's own workqueue, so you don't need cryptd here.
On recent kernels this workqueue is multithreaded, so if the
crypto requests are send from multiple cpus it behaves well
parallelized. If only one cpu sends crypto requests, it is
serialized to this cpu. pcrypt could help here, but it still
lacks ablkcipher support what is needed for dm-crypt.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fwd: crypto accelerator driver problems

[Steffen Klassert]

On Wed, Oct 05, 2011 at 01:33:33PM +0330, Hamid Nassiby wrote:
 
 OK, I represented code as PSEUSO, just to simplify and concentrate problem's
 aspects ;),  (but it is also possible that I've concentrated it in a
 wrong way :D)
 This is my_cbc_encrypt code and functions it calls, bottom-up:
 
 int write_request(u8 *buff, unsigned int count)
 {
 
   u32  tlp_size = 32;
   struct my_dma_desc *desc_table = (struct my_dma_desc *)global_bar[0];
   tlp_size = (count/128) | (tlp_size  16);
   memcpy(g_mydev-rdmaBuf_va, buff, count);
   wmb();
 
   writel(cpu_to_le32(tlp_size),(desc_table-wdmaperf));
   wmb();
 
   while((readl(desc_table-ddmacr) | 0x)!= 0x0101);/*wait for
   transfer compeltion*/
   return 0;
 }
 
  int my_transform(struct my_aes_op *op, int alg)
 {
 
   int  req_len, err;
   unsigned long iflagsq, tflag;
   u8 *req_buf = NULL, *res_buf = NULL;
   alg_operation operation;
   if (op-len == 0)
   return 0;
   operation = !(op-dir);
 
   create_request(alg, op-mode, operation, 0, op-key,
 op-iv, op-src, op-len, req_buf, req_len); /*add
   header to original request and copy it to req_buf*/
 
   spin_lock_irqsave(glock, tflag);
   
   write_request(req_buf, req_len);/*now req_buf is sent to device
   , device en/decrypts request and writes the
   the result to a fixed dma mapped address*/
   if (err){
   printk(KERN_EMERGError WriteReuest:errcode=%d\n, err);
   //handle exception (never occured)
   }
   kfree(req_buf);
   req_buf = NULL;
 
   memcpy(op-dst, (g_mydev-wdmaBuf_va, op-len);/*copy result 
 from
fixed coherent dma mapped memory to destination*/
   spin_unlock_irqrestore(glock, tflag);
   
   return op-len;
 }
 
 static int
 my_cbc_encrypt(struct blkcipher_desc *desc,
 struct scatterlist *dst, struct scatterlist *src,
 unsigned int nbytes)
 {
   struct my_aes_op *op = crypto_blkcipher_ctx(desc-tfm);
   struct blkcipher_walk walk;
   int err, ret;
   unsigned long c2flag;
   if (unlikely(op-keylen != AES_KEYSIZE_128))
   return fallback_blk_enc(desc, dst, src, nbytes);
 
 
   blkcipher_walk_init(walk, dst, src, nbytes);
   err = blkcipher_walk_virt(desc, walk);
   op-iv = walk.iv;
 
   while((nbytes = walk.nbytes)) {
 
   op-src = walk.src.virt.addr,
   op-dst = walk.dst.virt.addr;
   op-mode = AES_MODE_CBC;
   op-len = nbytes /*- (nbytes % AES_MIN_BLOCK_SIZE)*/;
   op-dir = AES_DIR_ENCRYPT;
   ret = my_transform(op, 0);
   nbytes -= ret;
   err = blkcipher_walk_done(desc, walk, nbytes);
   }
 
   return err;
 }
 

I can't tell much when looking at this code snippet. One guess would be
someone (maybe you) has set the CRYPTO_TFM_REQ_MAY_SLEEP flag, as
blkcipher_walk_done calls crypto_yield() which in turn might call
schedule() if this flag is set. prcypt removes this flag explicit.

 
 Did you turn BHs off, to prevent deadlocks  between your workqueues and
 network's softirqs?
 If there is any other thing that will help, I am pleased to hear.
 

Basically, the bottom halves are off to keep up with the network softirqs.
They run with much higher priority and would interrupt the parallel
workers frequently.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 04/17] crypto: Add userspace configuration API

[Steffen Klassert]

On Wed, Sep 21, 2011 at 11:02:38AM +0200, Steffen Klassert wrote:
 +
 +static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 +{
 + struct nlattr *attrs[CRYPTOCFGA_MAX+1];
 + struct crypto_link *link;
 + int type, err;
 +
 + type = nlh-nlmsg_type;
 + if (type  CRYPTO_MSG_MAX)
 + return -EINVAL;
 +
 + type -= CRYPTO_MSG_BASE;
 + link = crypto_dispatch[type];
 +
 + if (security_netlink_recv(skb, CAP_NET_ADMIN))
 + return -EPERM;
 +

I'm just wondering whether CAP_NET_ADMIN is the right capability to
use here? Do you think we can keep it like that, or would it be better
to define a new CAP_CRYPTO_ADMIN capability?
 
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 00/17] crypto user configuration api

[Steffen Klassert]

This patchset adds a netlink based user configuration API for the crypto
layer, similar to the configuration API of xfrm.

The patchset is based on the current cryptodev tree.

A userspace tool that makes use of the configuration API is available at

https://sourceforge.net/projects/crconf/files/crconf-pre2.tar.gz

With this it is possible to instantiate certain algorithms by doing

crconf add driver cbc(aes-generic) type 4

or

crconf add driver cbc(aes-generic) type 4 priority 100

To remove a (form templates build and unused) algorithm with all subsequent
algorithms do

crconf del driver cbc(aes-generic) type 4

It is possible to update the priority of an algorithm by doing

crconf update driver cbc(aes-generic) type 4 priority 200

this updates the priority of this algorithm and removes all algorithms
on top.

Finally it is possible to print the instantiated crypto algorithms
similar to /proc/crypto by doing

crconf show all

This prints the algorithm informations of all instantiated algorithms
as long as the information fits into a netlink message.

Changes from v1:

- Removed the priority update functions.
- Fix algorithm information printing when build as module.
- Update the crconf tool according to the kernel changes.

Changes from v2:

- Use one structure for creating and basic querying of algorithms.
- Send the algorithm flags to userspace, so the userspace can
  check for things like passed selftest, async algorithms etc.
- Update the crconf tool according to the kernel changes.
- Add some priority update functions. We need to be able to update
  the priority of algorithms, as we can't delete core algorithms like
  aes-generic. When we update the priority of an algorithm, we remove
  all algorithms on top.

Changes from v3:

- Remove the priority field from struct crypto_user_alg and use the
  existing netlink attribute to send the priority value to userspace.
- Update the crconf tool according to the kernel changes.
- Don't distinguish between netlink attributes that use the same
  type value.

Steffen
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 01/17] crypto: Add a flag to identify crypto instances

[Steffen Klassert]

The upcomming crypto user configuration api needs to identify
crypto instances. This patch adds a flag that is set if the
algorithm is an instance that is build from templates.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/algapi.c|1 +
 include/linux/crypto.h |5 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index c3cf1a6..6fd9bcf 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -493,6 +493,7 @@ int crypto_register_instance(struct crypto_template *tmpl,
goto err;
 
inst-alg.cra_module = tmpl-module;
+   inst-alg.cra_flags |= CRYPTO_ALG_INSTANCE;
 
down_write(crypto_alg_sem);
 
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index e5e468e..de9adec 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -72,6 +72,11 @@
 #define CRYPTO_ALG_TESTED  0x0400
 
 /*
+ * Set if the algorithm is an instance that is build from templates.
+ */
+#define CRYPTO_ALG_INSTANCE0x0800
+
+/*
  * Transform masks and values (for crt_flags).
  */
 #define CRYPTO_TFM_REQ_MASK0x000fff00
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 03/17] crypto: Export crypto_remove_final

[Steffen Klassert]

The upcomming crypto usrerspace configuration api needs
to remove the spawns on top on an algorithm, so export
crypto_remove_final.

Signed-off-by: Steffen Klassert steffen.klass...@secunet.com
---
 crypto/algapi.c   |5 ++---
 crypto/internal.h |1 +
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 1b54d74..54dd4e3 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -22,8 +22,6 @@
 
 #include internal.h
 
-static void crypto_remove_final(struct list_head *list);
-
 static LIST_HEAD(crypto_template_list);
 
 void crypto_larval_error(const char *name, u32 type, u32 mask)
@@ -321,7 +319,7 @@ unlock:
 }
 EXPORT_SYMBOL_GPL(crypto_alg_tested);
 
-static void crypto_remove_final(struct list_head *list)
+void crypto_remove_final(struct list_head *list)
 {
struct crypto_alg *alg;
struct crypto_alg *n;
@@ -331,6 +329,7 @@ static void crypto_remove_final(struct list_head *list)
crypto_alg_put(alg);
}
 }
+EXPORT_SYMBOL_GPL(crypto_remove_final);
 
 static void crypto_wait_for_test(struct crypto_larval *larval)
 {
diff --git a/crypto/internal.h b/crypto/internal.h
index b6dcb31..b865ca1 100644
--- a/crypto/internal.h
+++ b/crypto/internal.h
@@ -88,6 +88,7 @@ void crypto_alg_tested(const char *name, int err);
 
 void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
  struct crypto_alg *nalg);
+void crypto_remove_final(struct list_head *list);
 void crypto_shoot_alg(struct crypto_alg *alg);
 struct crypto_tfm *__crypto_alloc_tfm(struct crypto_alg *alg, u32 type,
  u32 mask);
-- 
1.7.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html