On Mon, Jan 01, 2024 at 12:24:59PM +0100, Markus Elfring wrote: > … > >> Thus use another label. > … > >> --- > >> net/sunrpc/auth_gss/gss_krb5_crypto.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > … > > As has undoubtedly been pointed out in other forums, calling kfree() > > with a NULL argument is perfectly valid. > > The function call “kfree(NULL)” is not really useful for error/exception handling > while it is tolerated at various source code places. > > > > Since this small GFP_KERNEL > > allocation almost never fails, it's unlikely this change is going to > > make any difference except for readability. > > I became curious if development interests can grow for the usage of > an additional label. > https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources > > > > Now if we want to clean up the error flows in here to look more > > idiomatic, how about this: > … > > +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c > … > > @@ -970,8 +970,9 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher, > > > > out_free_ahash: > > ahash_request_free(req); > > -out_free_mem: > > +out_free_iv: > > kfree(iv); > > +out_free_cksumdata: > > kfree_sensitive(checksumdata); > … > > I find it nice that you show another possible adjustment of corresponding identifiers. I got curious, and tried using a static const buffer instead of allocating one that always contains zeroes. The following compiles and passes the SunRPC GSS Kunit tests: commit 52d614882af007630072857b6b95a6d0fda23c1c Author: Chuck Lever <chuck.lever@xxxxxxxxxx> AuthorDate: Mon Jan 1 11:37:45 2024 -0500 Commit: Chuck Lever <chuck.lever@xxxxxxxxxx> CommitDate: Mon Jan 1 11:47:06 2024 -0500 SUNRPC: Use a static buffer for the checksum initialization vector Allocating and zeroing a buffer during every call to krb5_etm_checksum() is inefficient. Instead, set aside a static buffer that is the maximum crypto block size, and use a portion (or all) of that. Reported-by: Markus Elfring <Markus.Elfring@xxxxxx> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c index d2b02710ab07..82dc1eddf050 100644 --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c @@ -921,6 +921,8 @@ gss_krb5_aes_decrypt(struct krb5_ctx *kctx, u32 offset, u32 len, * Caller provides the truncation length of the output token (h) in * cksumout.len. * + * Note that for RPCSEC, the "initial cipher state" is always all zeroes. + * * Return values: * %GSS_S_COMPLETE: Digest computed, @cksumout filled in * %GSS_S_FAILURE: Call failed @@ -931,22 +933,19 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher, int body_offset, struct xdr_netobj *cksumout) { unsigned int ivsize = crypto_sync_skcipher_ivsize(cipher); + static const u8 iv[GSS_KRB5_MAX_BLOCKSIZE]; struct ahash_request *req; struct scatterlist sg[1]; - u8 *iv, *checksumdata; + u8 *checksumdata; int err = -ENOMEM; checksumdata = kmalloc(crypto_ahash_digestsize(tfm), GFP_KERNEL); if (!checksumdata) return GSS_S_FAILURE; - /* For RPCSEC, the "initial cipher state" is always all zeroes. */ - iv = kzalloc(ivsize, GFP_KERNEL); - if (!iv) - goto out_free_mem; req = ahash_request_alloc(tfm, GFP_KERNEL); if (!req) - goto out_free_mem; + goto out_free_cksumdata; ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL); err = crypto_ahash_init(req); if (err) @@ -970,8 +969,7 @@ u32 krb5_etm_checksum(struct crypto_sync_skcipher *cipher, out_free_ahash: ahash_request_free(req); -out_free_mem: - kfree(iv); +out_free_cksumdata: kfree_sensitive(checksumdata); return err ? GSS_S_FAILURE : GSS_S_COMPLETE; } I haven't tested this with an actual sec=krb5[ip] workload yet. -- Chuck Lever
