Fixing ICP memleak introduced in #4760 #5265
Conversation
The ICP requires destructors to for each crypto module that is added. These do not necessarily exist in Illumos because they assume that these modules can never be unloaded from the kernel. Some of this cleanup code was missed when openzfs#4760 was merged, resulting in leaks. This patch simply fixes that.
|
@tcaputi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tonyhutter to be a potential reviewer. |
|
Good catch. LGTM |
| @@ -231,6 +231,19 @@ skein_mod_init(void) | |||
|
|
|||
| int | |||
| skein_mod_fini(void) { | |||
| int ret; | |||
There was a problem hiding this comment.
[style] by convention the existing code prefers to name this kind of variable error rather than ret.
|
|
||
| if (skein_prov_handle != 0) { | ||
| if ((ret = crypto_unregister_provider(skein_prov_handle)) != | ||
| CRYPTO_SUCCESS) { |
There was a problem hiding this comment.
[style] This one's situation dependent but often it's clearer to pull the assignment out of the conditional in cases like this where the line wraps.
error = crypto_unregister_provider(skein_prov_handle);
if (error != CRYPTO_SUCCESS) {
}
There was a problem hiding this comment.
I styled the code like this in order to match the existing code of Illumos's KCF (and therefore the rest of the ICP). For instance, here is the _init() code from Illumos's sha2_mod.c. Would you still like me to change it?
There was a problem hiding this comment.
Ahh I see, that explains it. Well given that I'm OK with either styling. Either we're not going to be consistent with the other _mod_fini() styling, or with the styling in the rest of the skein_mod.c file. Objection withdrawn. LGTM.
| cmn_err(CE_WARN, | ||
| "skein _fini: crypto_unregister_provider() " | ||
| "failed (0x%x)", ret); | ||
| return (EBUSY); |
There was a problem hiding this comment.
Just for future reference in the core zfs.ko code most places where a const errno is returned the SET_ERROR() macro is used to log it. return (SET_ERROR(EBUSY));. That said, none of the existing icp code does this so let's avoid doing that here. No change needed.
The ICP requires destructors to for each crypto module that is added.
These do not necessarily exist in Illumos because they assume that
these modules can never be unloaded from the kernel. Some of this
cleanup code was missed when #4760 was merged, resulting in leaks.
This patch simply fixes that.