Re: [PATCH 2/4] x86/sev: Allocate request in TSC_INFO_REQ on stack

[Dave Hansen <dave.hansen@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

On 5/5/25 07:12, Alexey Kardashevskiy wrote:
> Allocate a 88 byte request structure on stack and skip needless
> kzalloc/kfree.

Could you maybe take a closer look at _all_ of these rather than poking
at them one at a time?

snp_guest_request_ioctl, for example, looks to be ~32 bytes. Why fix
'struct snp_guest_req' and leave an even worse offender?

Or, maybe just be done with it and convert them all over to __free().
Yeah, some of them don't need to be kmalloc(), but kmalloc()s are cheap
and consistency is nice, like in the attached patch.

It also wouldn't be awful to mix stack and kmalloc() allocations,
especially when the freeing semantics are the same for stack and
__free()-annotated allocations.

But it would be really nice to completely eliminate the goto mess.

[-- Attachment #2: snp_get_tsc_info-__free.patch --]
[-- Type: text/x-patch, Size: 2388 bytes --]

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index b0c1a7a57497..618eaae190e2 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -3174,41 +3174,32 @@ EXPORT_SYMBOL_GPL(snp_send_guest_request);
 
 static int __init snp_get_tsc_info(void)
 {
-	struct snp_guest_request_ioctl *rio;
-	struct snp_tsc_info_resp *tsc_resp;
-	struct snp_tsc_info_req *tsc_req;
-	struct snp_msg_desc *mdesc;
-	struct snp_guest_req *req;
+	struct snp_guest_request_ioctl *rio __free(kfree) = NULL;
+	struct snp_tsc_info_resp *tsc_resp  __free(kfree_sensitive) = NULL;
+	struct snp_tsc_info_req *tsc_req    __free(kfree) = NULL;
+	struct snp_guest_req *req	    __free(kfree) = NULL;
+	struct snp_msg_desc *mdesc	    __free(snp_msg_free) = NULL;
 	int rc = -ENOMEM;
 
-	tsc_req = kzalloc(sizeof(*tsc_req), GFP_KERNEL);
-	if (!tsc_req)
-		return rc;
-
 	/*
 	 * The intermediate response buffer is used while decrypting the
 	 * response payload. Make sure that it has enough space to cover
 	 * the authtag.
 	 */
 	tsc_resp = kzalloc(sizeof(*tsc_resp) + AUTHTAG_LEN, GFP_KERNEL);
-	if (!tsc_resp)
-		goto e_free_tsc_req;
-
+	tsc_req = kzalloc(sizeof(*tsc_req), GFP_KERNEL);
 	req = kzalloc(sizeof(*req), GFP_KERNEL);
-	if (!req)
-		goto e_free_tsc_resp;
-
 	rio = kzalloc(sizeof(*rio), GFP_KERNEL);
-	if (!rio)
-		goto e_free_req;
+	if (!(tsc_resp && tsc_req && req && rio))
+		return rc;
 
 	mdesc = snp_msg_alloc();
 	if (IS_ERR_OR_NULL(mdesc))
-		goto e_free_rio;
+		return rc;
 
 	rc = snp_msg_init(mdesc, snp_vmpl);
 	if (rc)
-		goto e_free_mdesc;
+		return rc;
 
 	req->msg_version = MSG_HDR_VER;
 	req->msg_type = SNP_MSG_TSC_INFO_REQ;
@@ -3221,7 +3212,7 @@ static int __init snp_get_tsc_info(void)
 
 	rc = snp_send_guest_request(mdesc, req, rio);
 	if (rc)
-		goto e_request;
+		return rc;
 
 	pr_debug("%s: response status 0x%x scale 0x%llx offset 0x%llx factor 0x%x\n",
 		 __func__, tsc_resp->status, tsc_resp->tsc_scale, tsc_resp->tsc_offset,
@@ -3235,20 +3226,6 @@ static int __init snp_get_tsc_info(void)
 		rc = -EIO;
 	}
 
-e_request:
-	/* The response buffer contains sensitive data, explicitly clear it. */
-	memzero_explicit(tsc_resp, sizeof(*tsc_resp) + AUTHTAG_LEN);
-e_free_mdesc:
-	snp_msg_free(mdesc);
-e_free_rio:
-	kfree(rio);
-e_free_req:
-	kfree(req);
- e_free_tsc_resp:
-	kfree(tsc_resp);
-e_free_tsc_req:
-	kfree(tsc_req);
-
 	return rc;
 }