[PATCH v10 08/24] virt: sev-guest: Take mutex in snp_send_guest_request()

Nikunj A Dadhania posted 24 patches 1 year, 5 months ago
There is a newer version of this series
[PATCH v10 08/24] virt: sev-guest: Take mutex in snp_send_guest_request()
Posted by Nikunj A Dadhania 1 year, 5 months ago
SNP command mutex is used to serialize access to the shared buffer, command
handling and message sequence number races.

As part of the preparation for moving SEV guest driver common code and
making mutex private, take the mutex in snp_send_guest_request() instead of
snp_guest_ioctl(). This will result in locking behavior change as detailed
below:

Current locking behaviour:

    snp_guest_ioctl()
      mutex_lock(&snp_cmd_mutex)
        get_report()/get_derived_key()/get_ext_report()
          snp_send_guest_request()
    	...
      mutex_unlock(&snp_cmd_mutex)

New locking behaviour:

    snp_guest_ioctl()
      get_report()/get_derived_key()/get_ext_report()
        snp_send_guest_request()
           guard(mutex)(&snp_cmd_mutex)
             ...

Remove multiple lockdep check in the sev-guest driver as they are redundant
now.

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index fcd61df08702..ed00c21ca821 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -122,8 +122,6 @@ static u64 snp_get_msg_seqno(struct snp_guest_dev *snp_dev)
 {
 	u64 count;
 
-	lockdep_assert_held(&snp_cmd_mutex);
-
 	count = snp_dev->secrets->os_area.msg_seqno[snp_dev->vmpck_id] + 1;
 
 	/*
@@ -345,6 +343,8 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
 	u64 seqno;
 	int rc;
 
+	guard(mutex)(&snp_cmd_mutex);
+
 	/* Get message sequence and verify that its a non-zero */
 	seqno = snp_get_msg_seqno(snp_dev);
 	if (!seqno)
@@ -399,8 +399,6 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 	struct snp_report_resp *report_resp;
 	int rc, resp_len;
 
-	lockdep_assert_held(&snp_cmd_mutex);
-
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
@@ -447,8 +445,6 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
 	/* Response data is 64 bytes and max authsize for GCM is 16 bytes. */
 	u8 buf[64 + 16];
 
-	lockdep_assert_held(&snp_cmd_mutex);
-
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
@@ -499,8 +495,6 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	sockptr_t certs_address;
 	int ret, resp_len;
 
-	lockdep_assert_held(&snp_cmd_mutex);
-
 	if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data))
 		return -EINVAL;
 
@@ -596,12 +590,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
 	if (!input.msg_version)
 		return -EINVAL;
 
-	mutex_lock(&snp_cmd_mutex);
-
 	/* Check if the VMPCK is not empty */
 	if (is_vmpck_empty(snp_dev)) {
 		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
-		mutex_unlock(&snp_cmd_mutex);
 		return -ENOTTY;
 	}
 
@@ -626,8 +617,6 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
 		break;
 	}
 
-	mutex_unlock(&snp_cmd_mutex);
-
 	if (input.exitinfo2 && copy_to_user(argp, &input, sizeof(input)))
 		return -EFAULT;
 
@@ -724,8 +713,6 @@ static int sev_svsm_report_new(struct tsm_report *report, void *data)
 	man_len = SZ_4K;
 	certs_len = SEV_FW_BLOB_MAX_SIZE;
 
-	guard(mutex)(&snp_cmd_mutex);
-
 	if (guid_is_null(&desc->service_guid)) {
 		call_id = SVSM_ATTEST_CALL(SVSM_ATTEST_SERVICES);
 	} else {
@@ -860,8 +847,6 @@ static int sev_report_new(struct tsm_report *report, void *data)
 	if (!buf)
 		return -ENOMEM;
 
-	guard(mutex)(&snp_cmd_mutex);
-
 	/* Check if the VMPCK is not empty */
 	if (is_vmpck_empty(snp_dev)) {
 		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
-- 
2.34.1
Re: [PATCH v10 08/24] virt: sev-guest: Take mutex in snp_send_guest_request()
Posted by Borislav Petkov 1 year, 5 months ago
On Fri, Jun 21, 2024 at 06:08:47PM +0530, Nikunj A Dadhania wrote:
> SNP command mutex is used to serialize access to the shared buffer, command
> handling and message sequence number races.

serialize access to ... races?

Needs re-formulation.

> As part of the preparation for moving SEV guest driver common code and
> making mutex private, take the mutex in snp_send_guest_request() instead of
> snp_guest_ioctl(). This will result in locking behavior change as detailed
> below:
> 
> Current locking behaviour:
> 
>     snp_guest_ioctl()
>       mutex_lock(&snp_cmd_mutex)
>         get_report()/get_derived_key()/get_ext_report()
>           snp_send_guest_request()
>     	...
>       mutex_unlock(&snp_cmd_mutex)
> 
> New locking behaviour:
> 
>     snp_guest_ioctl()
>       get_report()/get_derived_key()/get_ext_report()
>         snp_send_guest_request()
>            guard(mutex)(&snp_cmd_mutex)
>              ...

Why is it ok to grab the mutex in snp_send_guest_request()?

Folks need to learn to stop spelling out what the patch does but WHY it
does it and WHY is it ok?!?

> Remove multiple lockdep check in the sev-guest driver as they are redundant
> now.

More "what" redundancy.

"The new locking region covers <bla> and that is ok because of <foo>."

This is what your commit message should say.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette