Page MenuHomeFreeBSD

crypto(9): Use a more specific error code when a capable driver is not found
ClosedPublic

Authored by cem on Sep 22 2017, 4:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 9:54 AM
Unknown Object (File)
Jan 17 2024, 4:20 AM
Unknown Object (File)
Jan 17 2024, 2:03 AM
Unknown Object (File)
Jan 3 2024, 9:37 AM
Unknown Object (File)
Dec 20 2023, 2:04 AM
Unknown Object (File)
Nov 29 2023, 3:02 PM
Unknown Object (File)
Nov 26 2023, 3:52 PM
Unknown Object (File)
Nov 10 2023, 11:51 AM
Subscribers
None

Details

Summary

When crypto_newsession() is given a request for an unsupported
capability, raise a more specific error than EINVAL.

This allows cryptotest.py to skip some HMAC tests that a driver does not
support.

Test Plan

Ran cryptotest.py with a SHA driver that only supports SHA1 and 256. SHA1 and
256 test cases are executed and pass. Others are skipped.

Diff Detail

Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 11671
Build 12017: arc lint + arc unit

Event Timeline

I was going to say that you needed to update crypto(4) but it doesn't have an ERRORS section yet. :(

Note that using this should also let you remove the hardcoded lists of supported "cipher groups" for device names and all the 'skipif' stuff. Do the tests report as FAIL or SKIP when you raise an exception like this?

sys/opencrypto/crypto.c
466

Why not change this instead? I think it will be more consistent and less confusing to have 'EOPNOTSUPP' mean "driver doesn't support algorithm or could not find a driver that does" than to use EINVAL still for the case that 'crid' was a wildcard and we couldn't find a driver that supports the algorithm.

In D12451#258445, @jhb wrote:

Note that using this should also let you remove the hardcoded lists of supported "cipher groups" for device names and all the 'skipif' stuff.

Potentially, although I wouldn't do that. The goal is to allow running any supported NIST-KAT HMAC tests against an HMAC-supporting driver. For example, sha_sse only supports SHA1 and SHA256, while the NIST-KAT suite includes SHA224, 384, and 512 vectors as well. It seems wasteful to pummel an AES driver with a whole suite of unsupported HMAC vectors.

Do the tests report as FAIL or SKIP when you raise an exception like this?

Uncaught exceptions result in failure. This change catches only EOPNOTSUPP error exceptions.

sys/opencrypto/crypto.c
466

I think it might be useful to distinguish the two cases with different error codes.

The "could not find a driver" case could be ENOENT or ENXIO or something.

It would be good to get a proper errors section added to the documentation.

tests/sys/opencrypto/cryptotest.py
290

I'd recommend moving this comment after the except so that it has sane indenting.

In D12451#258497, @jmg wrote:

It would be good to get a proper errors section added to the documentation.

Do you consider that an essential part of this change, or could that go in as a separate revision?

tests/sys/opencrypto/cryptotest.py
290

Seems sane and clear to me. Comments generally go before the line(s) they describe.

In D12451#258622, @cem wrote:
In D12451#258497, @jmg wrote:

It would be good to get a proper errors section added to the documentation.

Do you consider that an essential part of this change, or could that go in as a separate revision?

Any change that changes behavior, IMO, requires documentation updates as part of the change. W/o documentation, the chance is incomplete.

In D12451#258784, @jmg wrote:

Any change that changes behavior, IMO, requires documentation updates as part of the change. W/o documentation, the chance is incomplete.

The existing behavior is already undocumented. This isn't reducing the accuracy of the documentation.

I still prefer a single errno value (it's also a smaller code patch and is a bit simpler to read post-patch). I don't think adding the errors section is a hard requirement for this change though, that is an old bug.

tests/sys/opencrypto/cryptotest.py
290

I think it's a little clearer if it's inside the except as it is describing what the except clause does. Either that or before the try (as it is describing the reason for having a try/except at all), but of those two I prefer it inside the except.

cem planned changes to this revision.Sep 25 2017, 6:29 PM
In D12451#258930, @jhb wrote:

I still prefer a single errno value (it's also a smaller code patch and is a bit simpler to read post-patch).

Ok.

tests/sys/opencrypto/cryptotest.py
290

Yeah, I think that's backwards, but it seems to be consensus — will do.

cem marked 4 inline comments as done.
  • Return NOTSUPP for any missed capability
  • Move a comment below the line it describes by popular demand
cem retitled this revision from crypto(9): Use a more specific error code when a driver isn't capable to crypto(9): Use a more specific error code when a capable driver is not found.Sep 25 2017, 6:36 PM
cem edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Sep 25 2017, 8:02 PM

The change itself seems fine to me. I'll defer to others with respect to whether this is the right direction and whether the documentation needs to be in this revision.

I don't think it is necessarily inconsistent that the various cryptodev_newsession implementations return EINVAL for the corresponding cases where they don't support an algorithm. It may be worth a comment somewhere though.

I guess an alternative might have been to make a more fine-grained capability map for each driver in the test. This would be more of a pain to maintain, but on the positive side it would not require (undocumented) error interpretation, and it would allow checking for erroneous lack of support (as in, we want to test that $driver supports $algo and if it returns EINVAL/EOPNOTSUPP instead then we want to fail the test).

I don't think it is necessarily inconsistent that the various cryptodev_newsession implementations return EINVAL for the corresponding cases where they don't support an algorithm. It may be worth a comment somewhere though.

I think those could maybe even be turned into ASSERTs. It seems like crypto(4) should prevent drivers from being invoked with capabilities they do not advertise.

This revision was automatically updated to reflect the committed changes.