diff mbox series

crypto: marvell/cesa - Avoid empty transfer descriptor

Message ID aB8t1ZTVBexqGlcm@gondor.apana.org.au
State New
Headers show
Series crypto: marvell/cesa - Avoid empty transfer descriptor | expand

Commit Message

Herbert Xu May 10, 2025, 10:43 a.m. UTC 
On Sat, May 10, 2025 at 11:38:16AM +0200, Klaus Kudielka wrote:
>
> Patch applied on top.
> On the first attempt all self-tests passed, but on the second and third unfortunately not.

This patch fixes the extra zero-length TDMA descriptor, which even
if it doesn't fix the problem, is obviously the right thing to do.

---8<---
The user may set req->src even if req->nbytes == 0.  If there
is no data to hash from req->src, do not generate an empty TDMA
descriptor.

Fixes: db509a45339f ("crypto: marvell/cesa - add TDMA support")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 drivers/crypto/marvell/cesa/hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Klaus Kudielka May 14, 2025, 5:12 a.m. UTC | #1 
On Tue, 2025-05-13 at 17:20 +0800, Herbert Xu wrote:
> On Sun, May 11, 2025 at 06:39:43PM +0200, Klaus Kudielka wrote:
> > 
> > Here the log after modprobe, with the new printk patch:
> 
> Thanks.  I'm starting to get the feeling that the partial hash
> is corrupted.
> 
> Please apply this patch on top of the printk patch to confirm this.
> 
> Cheers,

drivers/crypto/marvell/cesa/hash.c: In function ‘mv_cesa_ahash_complete’:
drivers/crypto/marvell/cesa/hash.c:403:25: error: implicit declaration of function ‘HASH_FBREQ_ON_STACK’; did you mean ‘SHASH_DESC_ON_STACK’? [-Wimplicit-function-declaration]
  403 |                         HASH_FBREQ_ON_STACK(fbreq, ahashreq);
      |                         ^~~~~~~~~~~~~~~~~~~
      |                         SHASH_DESC_ON_STACK
drivers/crypto/marvell/cesa/hash.c:403:45: error: ‘fbreq’ undeclared (first use in this function)
  403 |                         HASH_FBREQ_ON_STACK(fbreq, ahashreq);
      |                                             ^~~~~
drivers/crypto/marvell/cesa/hash.c:403:45: note: each undeclared identifier is reported only once for each function it appears in
drivers/crypto/marvell/cesa/hash.c:405:25: error: implicit declaration of function ‘crypto_ahash_import_core’; did you mean ‘crypto_ahash_import’? [-Wimplicit-function-declaration]
  405 |                         crypto_ahash_import_core(fbreq, &state);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~
      |                         crypto_ahash_import
  CC [M]  drivers/crypto/marvell/cesa/tdma.o
drivers/crypto/marvell/cesa/hash.c:407:25: error: implicit declaration of function ‘crypto_ahash_export_core’; did you mean ‘crypto_ahash_export’? [-Wimplicit-function-declaration]
  407 |                         crypto_ahash_export_core(fbreq, &state);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~
      |                         crypto_ahash_export
make[9]: *** [scripts/Makefile.build:203: drivers/crypto/marvell/cesa/hash.o] Error 1
Klaus Kudielka May 15, 2025, 5:53 p.m. UTC | #2 
On Wed, 2025-05-14 at 13:14 +0800, Herbert Xu wrote:
> 
> Sorry, should've mentioned that this goes on top of the current
> cryptodev tree:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/log/
> 

Okay, so now I have two printk patches on top of the current cryptodev tree.

I have enabled CRYPTO_SELFTESTS

- Three successful reboot / modprobe marvell-cesa. All self-tests passed.
- But the whole self-tests sequence now take approx. 2 minutes, instead of 15 seconds with plain v6.15-rc5
- And the journal gets huge. 24k lines. I am attaching a gzipped version, hope this works.

Best regards, Klaus
Eric Biggers May 15, 2025, 6:21 p.m. UTC | #3 
On Thu, May 15, 2025 at 07:53:03PM +0200, Klaus Kudielka wrote:
> On Wed, 2025-05-14 at 13:14 +0800, Herbert Xu wrote:
> > 
> > Sorry, should've mentioned that this goes on top of the current
> > cryptodev tree:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/log/
> > 
> 
> Okay, so now I have two printk patches on top of the current cryptodev tree.
> 
> I have enabled CRYPTO_SELFTESTS
> 
> - Three successful reboot / modprobe marvell-cesa. All self-tests passed.
> - But the whole self-tests sequence now take approx. 2 minutes, instead of 15 seconds with plain v6.15-rc5
> - And the journal gets huge. 24k lines. I am attaching a gzipped version, hope this works.
> 
> Best regards, Klaus

CRYPTO_SELFTESTS now enables the full set of crypto self-tests, which for the
past 6 years have been needed to be run anyway to properly validate the drivers;
just developers often forgot to enable them because they were under a separate
kconfig option that had a confusing name.  So the longer test time is expected.
It's unfortunate that it takes 2 minutes on the platform you're testing (on most
platforms it's much faster), but presumably that is still okay since it's just a
development option?  People shouldn't be expecting to run these tests in
production kernels.  (But even if they are for some reason, the test time also
remains configurable via kernel command-line options.)

- Eric
Klaus Kudielka May 15, 2025, 6:45 p.m. UTC | #4 
On Thu, 2025-05-15 at 11:21 -0700, Eric Biggers wrote:
> 
> CRYPTO_SELFTESTS now enables the full set of crypto self-tests, which for the
> past 6 years have been needed to be run anyway to properly validate the drivers;
> just developers often forgot to enable them because they were under a separate
> kconfig option that had a confusing name.  So the longer test time is expected.
> It's unfortunate that it takes 2 minutes on the platform you're testing (on most
> platforms it's much faster), but presumably that is still okay since it's just a
> development option?  People shouldn't be expecting to run these tests in
> production kernels.  (But even if they are for some reason, the test time also
> remains configurable via kernel command-line options.)
> 
> - Eric

Probably it was only the massive amount of printk's which slowed the system down.

With the plain cryptodev tree I now see:

# modprobe marvell-cesa
# dmesg | tail
[    4.949108] mv88e6085 f1072004.mdio-mii:10 lan4: Link is Up - 1Gbps/Full - flow control rx/tx
[    4.949199] br0: port 2(lan4) entered blocking state
[    4.949210] br0: port 2(lan4) entered forwarding state
[   46.915547] marvell-cesa f1090000.crypto: CESA device successfully registered
[   47.077931] alg: skcipher: skipping comparison tests for mv-cbc-des3-ede because cbc(des3_ede-generic) is unavailable
[   47.096665] alg: skcipher: skipping comparison tests for mv-cbc-aes because cbc(aes-generic) is unavailable
[   47.103401] alg: skcipher: skipping comparison tests for mv-cbc-des because cbc(des-generic) is unavailable
[   47.121374] alg: skcipher: skipping comparison tests for mv-ecb-des because ecb(des-generic) is unavailable
[   47.133757] alg: skcipher: skipping comparison tests for mv-ecb-des3-ede because ecb(des3_ede-generic) is unavailable
[   47.138474] alg: skcipher: skipping comparison tests for mv-ecb-aes because ecb(aes-generic) is unavailable
# grep test /proc/crypto 
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed
selftest     : passed

...and the failing marvell-cesa self-tests seem to have magically disappeared.
I now had five successful reboot / modprobe marvell-cesa in a row.

Best regards, Klaus
Herbert Xu May 15, 2025, 11:25 p.m. UTC | #5 
On Thu, May 15, 2025 at 08:45:39PM +0200, Klaus Kudielka wrote:
>
> ...and the failing marvell-cesa self-tests seem to have magically disappeared.
> I now had five successful reboot / modprobe marvell-cesa in a row.

It's always unfortunate when a printk patch makes the problem
go away :)

Correntin, can you still reproduce the failures with the latest
cryptodev tree?

Thanks,
Herbert Xu May 16, 2025, 4:12 a.m. UTC | #6 
On Thu, May 15, 2025 at 07:53:03PM +0200, Klaus Kudielka wrote:
> 
> Okay, so now I have two printk patches on top of the current cryptodev tree.
> 
> I have enabled CRYPTO_SELFTESTS
> 
> - Three successful reboot / modprobe marvell-cesa. All self-tests passed.
> - But the whole self-tests sequence now take approx. 2 minutes, instead of 15 seconds with plain v6.15-rc5
> - And the journal gets huge. 24k lines. I am attaching a gzipped version, hope this works.

Something doesn't look right.  There are zero ahash lines in your
dmesg.  IOW all the output was from skcipher tests alone.

That could explain why ahash appears to be working.

What does /proc/crypto show after boot-up? Do the cesa ahash
algorithms show up as tested in there?

Cheers,
Herbert Xu May 16, 2025, 12:45 p.m. UTC | #7 
On Fri, May 16, 2025 at 02:41:02PM +0200, Corentin Labbe wrote:
>
> Yes I have still errors:

Great! Please apply my debugging patch and see if it shines any
light on the problem.

Thanks,
Klaus Kudielka May 16, 2025, 5:36 p.m. UTC | #8 
On Fri, 2025-05-16 at 12:12 +0800, Herbert Xu wrote:
> 
> Something doesn't look right.  There are zero ahash lines in your
> dmesg.  IOW all the output was from skcipher tests alone.
> 
> That could explain why ahash appears to be working.
> 
> What does /proc/crypto show after boot-up? Do the cesa ahash
> algorithms show up as tested in there?
> 
> Cheers,

Plain cryptodev tree, marvell-cesa as module.
Directly after boot, only the builtin crc32c-generic shows up.
After loading the module, the marvell-cesa ahash show up with "selftest : passed".
But whether they REALLY were tested, I can't say.

# cat /proc/crypto 
name         : crc32c
driver       : crc32c-generic
module       : kernel
priority     : 100
refcnt       : 1
selftest     : passed
internal     : no
type         : shash
blocksize    : 1
digestsize   : 4

# modprobe marvell-cesa
# cat /proc/crypto 
name         : hmac(sha256)
driver       : mv-hmac-sha256
module       : marvell_cesa
priority     : 0
refcnt       : 1
selftest     : passed
internal     : no
type         : ahash
async        : yes
blocksize    : 64
digestsize   : 32

name         : hmac(sha1)
driver       : mv-hmac-sha1
module       : marvell_cesa
priority     : 0
refcnt       : 1
selftest     : passed
internal     : no
type         : ahash
async        : yes
blocksize    : 64
digestsize   : 20

name         : hmac(md5)
driver       : mv-hmac-md5
module       : marvell_cesa
priority     : 0
refcnt       : 1
selftest     : passed
internal     : no
type         : ahash
async        : yes
blocksize    : 64
digestsize   : 16

name         : sha256
driver       : mv-sha256
module       : marvell_cesa
priority     : 0
refcnt       : 1
selftest     : passed
internal     : no
type         : ahash
async        : yes
blocksize    : 64
digestsize   : 32

name         : sha1
driver       : mv-sha1
module       : marvell_cesa
priority     : 0
refcnt       : 1
selftest     : passed
internal     : no
type         : ahash
async        : yes
blocksize    : 64
digestsize   : 20

name         : md5
driver       : mv-md5
module       : marvell_cesa
priority     : 0
refcnt       : 1
selftest     : passed
internal     : no
type         : ahash
async        : yes
blocksize    : 64
digestsize   : 16

name         : cbc(aes)
driver       : mv-cbc-aes
module       : marvell_cesa
priority     : 300
refcnt       : 1
selftest     : passed
internal     : no
type         : skcipher
async        : yes
blocksize    : 16
min keysize  : 16
max keysize  : 32
ivsize       : 16
chunksize    : 16
walksize     : 16
statesize    : 0

name         : ecb(aes)
driver       : mv-ecb-aes
module       : marvell_cesa
priority     : 300
refcnt       : 1
selftest     : passed
internal     : no
type         : skcipher
async        : yes
blocksize    : 16
min keysize  : 16
max keysize  : 32
ivsize       : 0
chunksize    : 16
walksize     : 16
statesize    : 0

name         : cbc(des3_ede)
driver       : mv-cbc-des3-ede
module       : marvell_cesa
priority     : 300
refcnt       : 1
selftest     : passed
internal     : no
type         : skcipher
async        : yes
blocksize    : 8
min keysize  : 24
max keysize  : 24
ivsize       : 8
chunksize    : 8
walksize     : 8
statesize    : 0

name         : ecb(des3_ede)
driver       : mv-ecb-des3-ede
module       : marvell_cesa
priority     : 300
refcnt       : 1
selftest     : passed
internal     : no
type         : skcipher
async        : yes
blocksize    : 8
min keysize  : 24
max keysize  : 24
ivsize       : 0
chunksize    : 8
walksize     : 8
statesize    : 0

name         : cbc(des)
driver       : mv-cbc-des
module       : marvell_cesa
priority     : 300
refcnt       : 1
selftest     : passed
internal     : no
type         : skcipher
async        : yes
blocksize    : 8
min keysize  : 8
max keysize  : 8
ivsize       : 8
chunksize    : 8
walksize     : 8
statesize    : 0

name         : ecb(des)
driver       : mv-ecb-des
module       : marvell_cesa
priority     : 300
refcnt       : 1
selftest     : passed
internal     : no
type         : skcipher
async        : yes
blocksize    : 8
min keysize  : 8
max keysize  : 8
ivsize       : 0
chunksize    : 8
walksize     : 8
statesize    : 0

name         : crc32c
driver       : crc32c-generic
module       : kernel
priority     : 100
refcnt       : 1
selftest     : passed
internal     : no
type         : shash
blocksize    : 1
digestsize   : 4

# dmesg | tail
[    4.883366] mv88e6085 f1072004.mdio-mii:10 lan2: configuring for phy/gmii link mode
[    4.884517] br0: port 3(lan4) entered blocking state
[    4.884527] br0: port 3(lan4) entered forwarding state
[   88.867746] marvell-cesa f1090000.crypto: CESA device successfully registered
[   89.034292] alg: skcipher: skipping comparison tests for mv-cbc-des because cbc(des-generic) is unavailable
[   89.045265] alg: skcipher: skipping comparison tests for mv-cbc-des3-ede because cbc(des3_ede-generic) is unavailable
[   89.052456] alg: skcipher: skipping comparison tests for mv-cbc-aes because cbc(aes-generic) is unavailable
[   89.099763] alg: skcipher: skipping comparison tests for mv-ecb-aes because ecb(aes-generic) is unavailable
[   89.099819] alg: skcipher: skipping comparison tests for mv-ecb-des3-ede because ecb(des3_ede-generic) is unavailable
[   89.099908] alg: skcipher: skipping comparison tests for mv-ecb-des because ecb(des-generic) is unavailable
diff mbox series

Patch

diff --git a/drivers/crypto/marvell/cesa/hash.c b/drivers/crypto/marvell/cesa/hash.c
index a46ae868110a..2200bc6a034f 100644
--- a/drivers/crypto/marvell/cesa/hash.c
+++ b/drivers/crypto/marvell/cesa/hash.c
@@ -650,7 +650,7 @@  static int mv_cesa_ahash_dma_req_init(struct ahash_request *req)
 	if (ret)
 		goto err_free_tdma;
 
-	if (iter.src.sg) {
+	if (iter.base.len > iter.src.op_offset) {
 		/*
 		 * Add all the new data, inserting an operation block and
 		 * launch command between each full SRAM block-worth of