crypto: Remove uses of CRYPTO_F_DONE
ClosedPublic
Actions

Authored by markj on Thu, May 1, 2:27 PM.

Details

Reviewers

jhb

Commits

rG2fa185f9bf59: crypto: Remove uses of CRYPTO_F_DONE

Summary

Previously OCF set CRYPTO_F_DONE prior to invoking the completion
callback, even if the request failed. This isn't particularly useful
and leads to bugs when consumers retry a failed request, since OCF also
asserts that CRYPTO_F_DONE is clear in crypto_dispatch(). (Really, OCF
should retry requests that fail with EAGAIN, but that's a larger
change.)

For now, just stop setting CRYPTO_F_DONE to simplify consumers (and fix
those which fail to clear the flag before retrying a request). In krb5,
keep using it, but set it in the callback. Fix buggy handling of EAGAIN
there as well. Panic if the request fails for a different reason, since
we can't signal it to upper layers.

PR: 286321

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 63992
Build 60876: arc lint + arc unit

Event Timeline

markj created this revision.Thu, May 1, 2:27 PM

Herald added subscribers: ziaee, delphij, imp. · View Herald TranscriptThu, May 1, 2:27 PM

markj requested review of this revision.Thu, May 1, 2:27 PM

Harbormaster completed remote builds in B63820: Diff 154623.Thu, May 1, 2:27 PM

jhb added inline comments.Sun, May 4, 12:49 AM

sys/kgssapi/krb5/kcrypto_aes.c
121–123	Doesn't this case still need to set CRYPTO_F_DONE? Another way this could be handled btw would be to clear `crp_opaque` to NULL when a request is completed rather than using this flag. That would let you axe the flag entirely I think?

Apply John's suggestion

Harbormaster completed remote builds in B63872: Diff 154793.Sun, May 4, 2:49 PM

markj added inline comments.Sun, May 4, 2:49 PM

sys/kgssapi/krb5/kcrypto_aes.c
121–123	I don't think we need to set the flag in the synchronous case, as the dispatch code only checks it in the async case. Clearing the pointer instead seems fine.

So maybe one commit to update the krb5 code to use crp_opaque = NULL and then you can just remove the flag entirely in a second commit? The changes look ok to me though (I'm just also fine with now removing the flag entirely)

markj retitled this revision from crypto: Remove most uses of CRYPTO_F_DONE to crypto: Remove uses of CRYPTO_F_DONE.Wed, May 7, 12:32 PM

Apply John's suggestion of splitting the diff into two.

Harbormaster completed remote builds in B63992: Diff 155053.Wed, May 7, 12:33 PM

In D50104#1145336, @jhb wrote:

So maybe one commit to update the krb5 code to use crp_opaque = NULL and then you can just remove the flag entirely in a second commit? The changes look ok to me though (I'm just also fine with now removing the flag entirely)

That is better, thanks. See D50238 for the krb5 code.

jhb accepted this revision.Thu, May 8, 6:27 PM

jhb added inline comments.