mv_cesa hash functions

mv_cesa hash functions

[Phil Sutter]

Hello,

On Wed, Feb 22, 2012 at 02:03:38PM +0100, Frank wrote:
> After doing some trials with hardware crypto offloading through usermode interfaces (af_alg and cryptodev) to Marvell CESA accelerated ciphers and hash functions with the 3.2.4 kernel's mv_cesa in Debian Wheezy on a Marvell Kirkwood system, I've noticed the following kernel output when I load the mv_cesa kernel module:
> 
> [490889.448060] alg: hash: Test 1 failed for mv-sha1
> [490889.452786] 00000000: c1 94 3f 2e a2 41 ce 88 d5 47 07 43 c4 a8 17 5d
> [490889.459368] 00000010: 77 e8 47 ca
> [490889.464321] alg: hash: Test 1 failed for mv-hmac-sha1
> [490889.469493] 00000000: 06 71 4d 7c cc cc b5 cf 1b d6 c7 ab d0 25 c4 21
> [490889.476068] 00000010: 66 0b 8e 70

I've tracked down the problem to commit 6ef8450, "crypto: mv_cesa - make
count_sgs() null-pointer proof". So apparently it was me who broke it.

The simpification introduced by that commit assumes a zero 'nbytes'
field in the ahash_request struct passed to mv_hash_final, but
crypto/testmgr.c calls crypto_ahash_update() and crypto_ahash_final()
without altering the ahash_request in between.

Herbert, please clarify: is it intended behaviour that ahash_alg's final
callback ignores possibly present data in the request? If I wanted to
finalise a hash operation with some final data, would I then use the
finup callback instead? (Note the implied question of the actual
difference between the two callbacks.)

If my assumptions are correct, fixing this issue would be as easy as
adding a call to ahash_request_set_crypt() to mv_hash_final(). Or
manually zeroing req->nbytes, but that's probably unclean. Anyway, if
the 'final' callback is always to ignore request data, why doesn't the
API do this already? At least mv_cesa could use a single function for
both callbacks then.

> Using SHA1 in a ssl/tls handshake fails in tests with mv_cesa loaded, which might be related to this.

You are using cryptodev-linux for that, right? If so, this should be
unrelated since cryptodev-linux always calls crypto_ahash_final() with
an empty request payload (from cryptodev_cipher.c):

| ahash_request_set_crypt(hdata->async.request, NULL, output, 0);
| 
| ret = crypto_ahash_final(hdata->async.request);

But you might suffer from another problem, which is only present on ARM
machines with VIVT cache and linux >= 2.6.37: due to commit f8b63c1,
"ARM: 6382/1: Remove superfluous flush_kernel_dcache_page()" which
prevents pages being flushed from inside the scatterlist iterator API.
This patch seems to introduce problems in other places (namely NFS), too
but I sadly did not have time to investigate this further. I will post a
possible (cryptodev-internal) solution to cryptodev-linux-devel at gna.org,
maybe this fixes the problem with openssl.

Greetings, Phil

mv_cesa dcache problem since 2.6.37 was: Re: mv_cesa hash functions

[Simon Baatz]

Hi,

Am 23.02.2012 19:34, schrieb Phil Sutter:
> But you might suffer from another problem, which is only present on
> ARM machines with VIVT cache and linux >= 2.6.37: due to commit
> f8b63c1, "ARM: 6382/1: Remove superfluous flush_kernel_dcache_page()"
> which prevents pages being flushed from inside the scatterlist
> iterator API. This patch seems to introduce problems in other places
> (namely NFS), too but I sadly did not have time to investigate this
> further. I will post a possible (cryptodev-internal) solution to
> cryptodev-linux-devel at gna.org, maybe this fixes the problem with
> openssl. Greetings, Phil 

since there has been no reaction on this, I would like to bring this
issue up again (I sadly don't have the expertise to investigate this
further...). The issue is not limited to cryptodev, but seems to be
either a problem with commit f8b63c1 or a problem in mv_cesa that was
uncovered by this commit. In the past, I also had massive problems
compiling on an NFS mounted file system when not reverting f8b63c1. I
currently can't reproduce this anymore under 3.4-rc5.

However, the following still happens on a 3.4-rc5 preempt kernel on arm
kirkwood (VIVT cache):

root at ww1:~# cryptsetup luksOpen /dev/sda2 c_sda2
Enter passphrase for /dev/sda2:
root at ww1:~# vgchange -a y ww1_1
  Volume group "ww1_1" not found
root at ww1:~# vgchange -a y ww1_1
Segmentation fault


Thus, the behavior of vgchange is unpredictable on a dm-crypt device
when using mv_cesa (btw. the machine needs to have no load to show this
behavior. I assume that the cache flushes caused by task switches make
it work under load).

Using the generic crypto modules, it works as expected:

root at ww1:~# cryptsetup luksClose c_sda2
root at ww1:~# rmmod mv_cesa
root at ww1:~# cryptsetup luksOpen /dev/sda2 c_sda2
Enter passphrase for /dev/sda2:
root at ww1:~# vgchange -a y ww1_1
  1 logical volume(s) in volume group "ww1_1" now active


After reverting f8b63c1, it works as expected using mv_cesa as well.


- Simon

mv_cesa dcache problem since 2.6.37 was: Re: mv_cesa hash functions

[Russell King - ARM Linux]

On Sun, May 06, 2012 at 12:49:30AM +0200, Simon Baatz wrote:
> Am 23.02.2012 19:34, schrieb Phil Sutter:
> > But you might suffer from another problem, which is only present on
> > ARM machines with VIVT cache and linux >= 2.6.37: due to commit
> > f8b63c1, "ARM: 6382/1: Remove superfluous flush_kernel_dcache_page()"
> > which prevents pages being flushed from inside the scatterlist
> > iterator API. This patch seems to introduce problems in other places
> > (namely NFS), too but I sadly did not have time to investigate this
> > further. I will post a possible (cryptodev-internal) solution to
> > cryptodev-linux-devel at gna.org, maybe this fixes the problem with
> > openssl. Greetings, Phil 

Well, the reason it has been removed is as follows:

When pages are allocated, they have a special flag bit called PG_arch_1
cleared by the page allocator.

We used to use PG_arch_1 to indicate that the page contained dirty cache
lines - so by default, the page was marked clean.

Normal kernel behaviour upon allocating a page into the page cache is to
read data off the disk into the new page cache page, calling
flush_kernel_dcache_page() in the process.  This would ensure that data
would be pushed out of the cache lines associated with the kernel mapping
of the page into RAM.

This means when we map the page into userspace, we can be sure that the new
mapping will see the data written there.  Moreover, when we place a page
into a user process, we check this PG_arch_1 bit, and if it's set, we
flush the kernel mapping cache lines at this point as well.

Patch 6379/1 changed this behaviour: PG_arch_1 being _clear_ now indicates
that the page contains dirty data.

What this means is that when a page is placed into userspace, we check the
PG_arch_1 bit.  If it is clear, we flush the kernel mapping for the page
to push dirty data out to RAM.  This makes the flush in
flush_kernel_dcache_page() entirely redundant.

> since there has been no reaction on this, I would like to bring this
> issue up again (I sadly don't have the expertise to investigate this
> further...). The issue is not limited to cryptodev, but seems to be
> either a problem with commit f8b63c1 or a problem in mv_cesa that was
> uncovered by this commit.

Can you reproduce it with anything else?

It could be a problem with the way crypto stuff works - I've never had
any dealings with that subsystem, so I really can't comment.  If crypto
uses scatterlists, and walks them with the standard API, and uses
scatterlists with pages which are already mapped into userspace, then
I can see how the above commit would make things go wrong.

So, without knowledge of the crypto subsystem, I'm not sure I can help
sort this out.

If it's possible to reproduce with NFS, and it seems sorted in the latest
kernel, that's probably because something changed with NFS - NFS did for
a time have issues on ARM for various reasons (because of remapping kernel
memory into vmalloc space and expecting it to be DMA coherent...) and that
got fixed.  Whether that's why you're not seeing problems with v3.4-rc5,
I couldn't be sure unless you did a bisection between the bad and good
kernel versions.  That would at least allow us to confirm that that issue
has been properly resolved.

Thanks.

mv_cesa dcache problem since 2.6.37 was: Re: mv_cesa hash functions

[Phil Sutter]

Hi,

On Sun, May 06, 2012 at 01:25:06PM +0100, Russell King - ARM Linux wrote:
> On Sun, May 06, 2012 at 12:49:30AM +0200, Simon Baatz wrote:
> > since there has been no reaction on this, I would like to bring this
> > issue up again (I sadly don't have the expertise to investigate this
> > further...). The issue is not limited to cryptodev, but seems to be
> > either a problem with commit f8b63c1 or a problem in mv_cesa that was
> > uncovered by this commit.
> 
> Can you reproduce it with anything else?
> 
> It could be a problem with the way crypto stuff works - I've never had
> any dealings with that subsystem, so I really can't comment.  If crypto
> uses scatterlists, and walks them with the standard API, and uses
> scatterlists with pages which are already mapped into userspace, then
> I can see how the above commit would make things go wrong.

This is quite exactly what happens. Cryptodev uses get_user_pages to get
the pages belonging to the passed userspace addresses, fills
scatterlists with that information and passes them on to the crypto API.
Mv_cesa.c (at least) then uses the scatterlist mapping iterator to feed
the data chunk by chunk to the crypto engine. Upon completion, cryptodev
simply releases the user pages again by using SetPageDirty() and
page_cache_release(). This has proven unreliable, and the solution I
found was to additionally call flush_dcache_page() before returning the
pages to userspace.

> So, without knowledge of the crypto subsystem, I'm not sure I can help
> sort this out.

What would you recommend for iterating over scatterlists with mapped
pages instead? Since the above commit, neither the mapping iterator API
nor the simple sg_next() seem to be suitable anymore.

Greetings, Phil

mv_cesa dcache problem since 2.6.37 was: Re: mv_cesa hash functions

[Simon Baatz]

On Sun, May 06, 2012 at 01:25:06PM +0100, Russell King - ARM Linux wrote:
> On Sun, May 06, 2012 at 12:49:30AM +0200, Simon Baatz wrote:
> > Am 23.02.2012 19:34, schrieb Phil Sutter:
> > > But you might suffer from another problem, which is only present on
> > > ARM machines with VIVT cache and linux >= 2.6.37: due to commit
> > > f8b63c1, "ARM: 6382/1: Remove superfluous flush_kernel_dcache_page()"
> > > which prevents pages being flushed from inside the scatterlist
> > > iterator API. This patch seems to introduce problems in other places
> > > (namely NFS), too but I sadly did not have time to investigate this
> > > further. I will post a possible (cryptodev-internal) solution to
> > > cryptodev-linux-devel at gna.org, maybe this fixes the problem with
> > > openssl. Greetings, Phil 
> 
> Well, the reason it has been removed is as follows:
> 

Thanks for the explanation. This helped a lot.

> > since there has been no reaction on this, I would like to bring this
> > issue up again (I sadly don't have the expertise to investigate this
> > further...). The issue is not limited to cryptodev, but seems to be
> > either a problem with commit f8b63c1 or a problem in mv_cesa that was
> > uncovered by this commit.
> 
> Can you reproduce it with anything else?

No, I only have issues with mv_cesa (and NFS in the past, see below).
 
> It could be a problem with the way crypto stuff works - I've never had
> any dealings with that subsystem, so I really can't comment.  If crypto
> uses scatterlists, and walks them with the standard API, and uses
> scatterlists with pages which are already mapped into userspace, then
> I can see how the above commit would make things go wrong.

As Phil explained in his mail, this seems to be what happens. I have
another question on top of his.  mv_cesa uses SG_MITER_TO_SG, whose
function was described in the respective commit as:

commit 6de7e356faf54aa75de5b624bbce28a5b776dfa8
Author: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Date:   Thu Jun 18 10:19:12 2009 +0200

    lib/scatterlist: add a flags to signalize mapping direction
    sg_miter_start() is currently unaware of the direction of the copy
    process (to or from the scatter list). It is important to know the
    direction because the page has to be flushed in case the data written
    is seen on a different mapping in user land on cache incoherent
    architectures.

Thus, I would have expected that the scatterlist iterators do the
necessary "magic" in the userspace mapped case.  Apparently, this is
not the case with the changes you describe above.  

Is each driver supposed to handle this case or can/should this be handled
in a generic fashion by scatterlists?

> If it's possible to reproduce with NFS, and it seems sorted in the latest
> kernel, that's probably because something changed with NFS - NFS did for
> a time have issues on ARM for various reasons (because of remapping kernel
> memory into vmalloc space and expecting it to be DMA coherent...) and that
> got fixed.  Whether that's why you're not seeing problems with v3.4-rc5,
> I couldn't be sure unless you did a bisection between the bad and good
> kernel versions.  That would at least allow us to confirm that that issue
> has been properly resolved.

I had no idea that I reverted f8b63c1 for so long, but you are right
again.  The merge b9d919a4ac6cf031b8e065f82ad8f1b0c9ed74b1 (in
2.6.38) containing:

NFS: Don't use vm_map_ram() in readdir

fixes my test case.


- Simon