crypto: Remove the return value from crypto_dispatch(_async)()

[markj]

Summary

It's always zero, so we can simplify error handling in consumers. No
functional change intended.

[markj]

sys/kgssapi/krb5/kcrypto_aes.c

135

I'm not sure this is right, I think CRYPTO_F_DONE will always be set at this point...

338

This if should probably be a while.

[kp]

Do we need a __FreeBSD_version bump for this?

sys/geom/eli/g_eli_crypto.c

101

I think we want to keep that 'error = crp->crp_etype;'

sys/opencrypto/crypto.c

1431

Should we be asserting that the result is 0 or ERESTART here?
Ah, that's D35382 and not included in this patch.

[markj]

In D35383#802177, @kp wrote:

Do we need a __FreeBSD_version bump for this?

I think we can get away without one: on 13.0 and later, crypto_dispatch() only ever returns 0 (modulo driver bugs), so consumers can do something like this:

#if __FreeBSD_version < 1300000
error = crypto_dispatch(...);
if (error != 0) {
    ...
}
#else
crypto_dispatch(...);
#endif

For instance, OpenZFS supports only 13.0+ and so can avoid having any ifdefs at all.

sys/geom/eli/g_eli_crypto.c

101

Indeed, thank you.

[markj]

• Fix a bug in the GELI modifications.

[jhb]

sys/geom/eli/g_eli.c

241

This function always returns 0 now.

sys/kgssapi/krb5/kcrypto_aes.c

135

I think if you fix the code below to use while then you can just drop the mutex entirely and just do the wakeup() unconditionally.

However, we should probably return after calling crypto_dispatch() (and I don't know if we need to clear F_DONE when retrying? I hate the session retry stuff in OCF currently)

170–171

Old bug: should probably be 'while' here instead of if.

sys/opencrypto/crypto.c

1488

So this was double enqueueing before. :-/

sys/opencrypto/ktls_ocf.c

240

Yeah, this is what I mean for the kgssapi ones, should be clearing the flag and returning immediately after crypto_dispatch().

[mav]

In D35383#802555, @markj wrote:

For instance, OpenZFS supports only 13.0+ and so can avoid having any ifdefs at all.

OpenZFS supports FreeBSD from 12.2-RELEASE, just from ports, not the base system. We at iX do not plan more releases with 12, but there are many OpenZFS + FreeBSD 12.2 systems in a shape of TrueNAS 12.0 in a wild, supporting the crypto. So this would be a breaking change and it would need ifdefs. BTW I've recently patched zfs_crypto_dispatch() upstream, it just waits for next merge to FreeBSD head. It would be good to commit the change there first and give it settle a bit before breaking the compatibility.

[markj]

In D35383#808704, @mav wrote:

In D35383#802555, @markj wrote:

For instance, OpenZFS supports only 13.0+ and so can avoid having any ifdefs at all.

OpenZFS supports FreeBSD from 12.2-RELEASE, just from ports, not the base system. We at iX do not plan more releases with 12, but there are many OpenZFS + FreeBSD 12.2 systems in a shape of TrueNAS 12.0 in a wild, supporting the crypto. So this would be a breaking change and it would need ifdefs.

Ok.

BTW I've recently patched zfs_crypto_dispatch() upstream, it just waits for next merge to FreeBSD head. It would be good to commit the change there first and give it settle a bit before breaking the compatibility.

It landed already, I need to rebase this patch. I'll patch OpenZFS first as you suggest, thanks.

[markj]

crp_callback's return value is also unused, I'm not sure if we should drop that as well.

sys/kgssapi/krb5/kcrypto_aes.c

135

We do need to clear F_DONE when retrying, if only to placate an assertion. I have a patch somewhere which adds a cryptop_reset() to handle this so that consumers don't have to open-code this. So this code needs to be fixed.

I don't see how the mutex can be dropped, isn't it needed to avoid lost wakeups?

[markj]

• Rebase through ZFS updates and the introduction of if_ovpn
• Simplify aes_crypto_cb()
• Simplify g_eli_crypto_rerun()

[jhb]

Oh, and yes, crp_callback should be changed to void.

sys/kgssapi/krb5/kcrypto_aes.c

130

This needs your new crypto_reset().

135

Hummm, I guess so. I dislike the F_DONE is set without the lock, but I guess it still works, yes.

[markj]

sys/kgssapi/krb5/kcrypto_aes.c

135

PR 265852 made me realize that this doesn't really work at all. CRYPTO_F_DONE is set as soon as the driver completes the request, but that can be well before the completion handler is invoked. In particular, with async completion, F_DONE is set before the "return worker" gets a hold of the cryptop. If aes_encrypt_1() observes that F_DONE is set without going to sleep, it immediately frees the cryptop, resulting in a use after free.

I think CRYPTO_F_DONE is effectively useless. krb5 is the only consumer that does anything with it. I propose removing CRYPTO_F_DONE outright, and instead reserving a flag bit for use by the consumer, which can set it once the request really is done. I would write that patch in such a way that it can be merged to stable/13, then rebase my outstanding crypto patches on top of that. Does that sound reasonable?

[jhb]

sys/kgssapi/krb5/kcrypto_aes.c

135

Oh, yeah, I think we should kill the flag. I'm not sure if we want a reserved flag or perhaps just a new field in struct cryptop that can be used by the consumer, something like 'crp_consumer_flags' maybe union'ed with 'crp_consumer_done'. Code that really cares about this should be using crp_opaque to manage this I think though and using that as the flag instead somehow, either by storing the flag in what crp_opaque points to, or one case I think we have is that the callback clears crp_opaque to NULL to mark it as "done".