Intel (R) QAT driver

[julianx.grajkowski_intel.com]

Summary

QAT in-tree driver ported from out-of-tree release available
from 01.org.

The driver exposes complete cryptography and data compression
API in the kernel and integrates with Open Cryto Framework.
Details of supported operations, devices and usage can be found
in man and on 01.org.

Patch co-authored by: Krzysztof Zdziarski <[e-mail]>
Patch co-authored by: Michal Jaraczewski <[e-mail]>
Patch co-authored by: Michal Gulbicki <[e-mail]>
Patch co-authored by: Julian Grajkowski <[e-mail]>
Patch co-authored by: Piotr Kasierski <[e-mail]>
Patch co-authored by: Adam Czupryna <[e-mail]>
Patch co-authored by: Konrad Zelazny <[e-mail]>
Patch co-authored by: Katarzyna Rucinska <[e-mail]>
Patch co-authored by: Lukasz Kolodzinski <[e-mail]>
Patch co-authored by: Zbigniew Jedlinski <[e-mail]>

Sponsored by: Intel Corporation

[jhb]

sys/dev/qat/qat/qat_ocf.c

498

Yes, looks good to me.

604

You probably don't need these zeroes. The API contract is that OCF allocates a zeroed qat_dsession object before it invokes the crypto_newsession callback (so it's also true that the crypto_get_driver_session() call above should never return NULL, as if OCF failed to allocate the qat_dsession object, it would return an error to the caller without invoking the crypto_newsession callback). crypto_get_driver_session() is just returning the pre-allocated qat_dsession object, not allocating it.

[markj]

Sorry for the delay.

There is a lock order reversal warning printed when unloading the driver:

lock order reversal: (sleepable after non-sleepable)
1st 0xfffffe00b32fd048 Instance MTX (Instance MTX, sleep mutex) @ /home/mark/src/freebsd/sys/dev/qat/qat/qat_ocf.c:1249
2nd 0xfffffe0073068360 qat cfg data (qat cfg data, sx) @ /home/mark/src/freebsd/sys/dev/qat/qat_common/adf_cfg.c:612

In particular, sx locks must always be acquired before mutexes.

Have you tried running the FreeBSD regression test suite with this driver loaded? There are some tests which exercise the OCF code fairly well: /usr/tests/sys/geom/class/eli (encrypted/auth'ed block device, good for testing concurrency), and tests/sys/netipsec. Some of the ELI tests fail with this driver loaded on a C3000, lots of session initialization failure messages get printed to the console. The IPSec tests fail too, and my kernel crashed with a use-after-free while running them. You might try using KASAN to help validate the code (the GENERIC-KASAN kernel configuration enables it).

This was with a C3000 device, if that matters.

sys/dev/qat/qat/qat_ocf.c

214

Is there any reason not to expose AES-CCM support?

527

Better to use M_NOWAIT here.

883

Shouldn't this check happen under the cyInstMtx lock?

894

I don't think it's a good idea to print this. If it happens frequently it'll spam the console.

936

I doubt it's a good idea to print this message unconditionally. It could in principle be triggered by some remote host.

958

Need to set crp->crp_etype and call crypto_done() before returning an error here.

980

What's the point of this tricky double-checking? We will acquire this mutex in qat_ocf_cookie_alloc() anyway.

1327

This is supposed to be an errno value, but the function will return something from an internal error namespace.

[markj]

I suspect there is some memory leak, too. If I run KTLS tests (/usr/tests/sys/kern/ktls_test) in a loop and check output from vmstat -m | grep qat after each run, the number of allocations from qat_ocf grows steadily.

[julianx.grajkowski_intel.com]

Thank you for all your comments @markj We will address them soon.

[julianx.grajkowski_intel.com]

Review comments addressed

[MichalX.Gulbicki_intel.com]

In D34632#791116, @markj wrote:

I suspect there is some memory leak, too. If I run KTLS tests (/usr/tests/sys/kern/ktls_test) in a loop and check output from vmstat -m | grep qat after each run, the number of allocations from qat_ocf grows steadily.

vmstat reported allocated and not released memory due to lack of proper sessions release handling in opencrypto/ktls_ocf.c code. Issue has been addressed in the mean time and vmstat is no longer reporting any issues after KTLS test suite execution.

sys/dev/qat/qat/qat_ocf.c

214

Support for CCM will be covered in further releases.

527

Changed as suggested, please let me know if I can mark this comment as "done"

604

Agree, changed as suggested, please let me know if I can mark this comment as "done"

883

qat_ocf_process method has been refactored to handle session initialization or update in the same critical zone as placing request on HW queues. Please let me know if I can mark this comment as "done"

894

Comment addressed. Please let me know if I can mark this comment as "done"

936

Comment addressed. Please let me know if I can mark this comment as "done".

958

Comment addressed. Please let me know if I can mark this comment as "done"

980

qat_ocf_process method has been refactored to handle session initialization or update in the same critical zone as placing request on HW queues and this "double-checking" code to acquire mutex has been removed. Please let me know if I can mark this comment as "done"

1327

Comment addressed. Please let me know if I can mark this comment as "done"

[pauamma_gundo.com]

Manual page still LGTM

[jhb]

A few notes about spurious checks for NULL. In cases where APIs guarantee non-NULL values (or existing program logic) I don't find these checks helpful (especially since other drivers do not include such checks) as it makes a reader have to pause and wonder what cases could result in these values being NULL.

sys/dev/qat/qat/qat_ocf.c

80

qat_dsession here will never be NULL, and I'm fairly certain qatInstance will never be NULL. For the latter you could use a KASSERT to check in debug kernels.

584

This will never be NULL, so this check is not needed.

591

This can also never be NULL. qat_instance is a pointer to a member of an array of objects, not a member of an array of pointers.

596

If this is never expected to be NULL then it would be better to test it as an assertion instead, e.g.

KASSERT(qat_instance->cyInstHandle != NULL, ("%s: no crypto instance available", __func__));

604

Sure, though you can remove the NULL check as it will never be NULL here.

659

This can never be NULL.

665

I believe qat_instance can never be NULL here as freesession is only called if the newsession hook returns success, and the newsession hook above always sets qatInstance to a non-NULL value.

882

This should never be NULL.

1025

M_WAITOK malloc calls never fail, so this will never be NULL.

1153

qat_softc is never NULL. (The caller already checks for non-NULL, though as noted in the caller below it's never non-NULL there either.)

1182

Always non-NULL.

1198

I believe you don't need this memset. The API from new-bus is that your softc is guaranteed to be zeroed at the start of your attach routine.

1221

qat_softc will always be non-NULL here.

1229

Also always non-NULL.

1262

This does not seem used?

1267

You will want to update this for recent changes in HEAD to remove qat_ocf_devclass and no longer pass it to this macro.

[markj]

I did some more testing and haven't been able to find any problems. I'm ok with the patch once John's comments are addressed.

sys/dev/qat/qat/qat_ocf.c

1025

There are many instances of this in the common code too.

[julianx.grajkowski_intel.com]

In D34632#805884, @markj wrote:

I did some more testing and haven't been able to find any problems. I'm ok with the patch once John's comments are addressed.

Hi @markj Thank you for review and testing, we are now doing some internal testing before pushing the patch with all the comments addressed.

[julianx.grajkowski_intel.com]

Addressed all comments from rpevious patch set.

[pauamma_gundo.com]

With another date bump, still LGTM.

share/man/man4/qat.4

4

Needs another bump

[julianx.grajkowski_intel.com]

sys/dev/qat/qat/qat_ocf.c

80

NULL check removed

584

NULL check removed

591

NULL check removed

596

NULL check removed

604

NULL check removed

659

NULL check removed

665

NULL check removed

1025

NULL check removed

1025

All NULL checks in common code removed

1153

NULL check removed

1182

NULL check removed

1198

memset removed

1221

NULL check removed

1229

NULL check removed

1262

Removed from all files

1267

DRIVER_MODULE* macros usage fixed in all files

[julianx.grajkowski_intel.com]

Updated date in man as suggested and fixed memory leak

[julianx.grajkowski_intel.com]

share/man/man4/qat.4

4

Date updated

[markj]

The OCF bits look ok to me. I'm going to do another round of testing, then send a heads-up to freebsd-current@. If there are no objections, I'll commit the driver to main without an MFC (i.e., it'll appear first in 14.0).

[markj]

So, the plan now is to keep a portion of the old driver around for C2XXX support. I made a change renaming the driver here: https://reviews.freebsd.org/D35817

Would it be possible to rebase the driver on top of that change? It's available in github: https://github.com/markjdb/freebsd/tree/ff/qat-intel

Also, please email me a copy of the (rebased) git commit so that I can apply it directly to the FreeBSD tree; I can download the patch from phabricator but then it loses the commit log message and authorship info. I'd like to import the change sometime next week. Thanks in advance.

[erj]

Should this be closed now?

[markj]

sys/dev/qat/qat/qat_ocf.c

430

I'm confused by this setting. Crypto requests have a crp_digest_start field in the request structure which specifies the buffer offset at which the digest is to be written or read from. However, the driver doesn't use it anywhere. For most transforms this is fine; each cookie has a contiguous buffer for the digest. But for GCM, this buffer is unused. So where does the digest get written, and how does the computed digest get written to the right offset into the crypto buffer?

In the old QAT driver the firmware didn't validate digests, it would just generate them and the driver would perform a comparison. symDpCallback() copies this logic from the old driver, but the behaviour is different for GCM, and it appears to cause memory corruption in some cases.

[MichalX.Gulbicki_intel.com]

sys/dev/qat/qat/qat_ocf.c

430

You are right - digestIsAppended flag assumes that MAC is placed right after payload
in the source buffer (FW limitation). In such case crp_digest_start is not taken into account
what could potentially cause memory corruption. To use HW MAC validation feature
and reflect crp_digest_start, we would have to copy provided MAC to HW buffer before
sending the request. However it could introduce additional complexity level due to the
fact - that OCF can specify the effective MAC length to be validate (csp_auth_mlen).
HW MAC validation feature doesn't provide possibility to set effective MAC length to validate - that's why we would have to handle request without and with csp_auth_mlen set in the different ways. To make it simple, we would suggest to treat AEAD and ETA in the same way - with MAC validation in the callback.

At this moment we do have HW MAC validation forced for GCM and CCM scenarios in the driver code. Removing this limitation seems to be not impacting however full testing cycle is required. Let us keep you posted and let us know if it's potentially acceptable approach.

[markj]

sys/dev/qat/qat/qat_ocf.c

430

I think that's acceptable. The old driver did verification in software for all cipher modes.

If you can provide a quick patch, just for testing, I'd be happy to verify that it fixes the problem that I see.

[jhb]

sys/dev/qat/qat/qat_ocf.c

430

For CCM you need the value of csp_auth_mlen to know how to compute the tag since the length is one of the values in block 0 of the MAC for CCM. However, you can choose to just reject sessions with an unsupported csp_auth_mlen value in your probesesssion hook. For example, you could just fail the probe of GCM sessions whose auth_mlen isn't 12. Then you can assume for GCM that you always have a full tag. Similarly for CCM you should reject sessions if they use an unsupported auth_mlen value.