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

[cem]

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.

[jhb]

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 ↗

(On Diff #33298)

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.

[cem]

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.

[cem]

sys/opencrypto/crypto.c

466 ↗

(On Diff #33298)

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.

[jmg]

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

tests/sys/opencrypto/cryptotest.py

290 ↗

(On Diff #33298)

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

[cem]

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 ↗

(On Diff #33298)

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

[jmg]

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.

[cem]

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.

[jhb]

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 ↗

(On Diff #33298)

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]

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 ↗

(On Diff #33298)

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

[cem]

• Return NOTSUPP for any missed capability
• Move a comment below the line it describes by popular demand

[jhb]

Thanks

[rlibby]

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).

[cem]

In D12451#259074, @rlibby wrote:

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.