[PATCH] iommu/amd: RCU-protect iommu_ga_log_notifier

[PATCH] iommu/amd: RCU-protect iommu_ga_log_notifier

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

iommu_poll_ga_log() accesses iommu_ga_log_notifier without an RCU
read-side critical section, so synchronize_rcu() in the unregister
path does not wait for an in-flight notifier callback.

Use rcu_assign_pointer() when updating the notifier and
rcu_dereference() under scoped_guard(rcu) when invoking it. Load the
pointer once to close the race with unregister.

Keep the RCU read-side critical section minimal: only the pointer load
and the indirect notifier call stay inside scoped_guard(rcu). Move
pr_debug and pr_err outside the section so that logging does not extend
the window that synchronize_rcu() must wait for.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/iommu/amd/iommu.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index a06cd61..a31464a 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1054,11 +1054,11 @@ static void iommu_poll_events(struct amd_iommu *iommu)
 }
 
 #ifdef CONFIG_IRQ_REMAP
-static int (*iommu_ga_log_notifier)(u32);
+static int (__rcu *iommu_ga_log_notifier)(u32 ga_tag);
 
 int amd_iommu_register_ga_log_notifier(int (*notifier)(u32))
 {
-	iommu_ga_log_notifier = notifier;
+	rcu_assign_pointer(iommu_ga_log_notifier, notifier);
 
 	/*
 	 * Ensure all in-flight IRQ handlers run to completion before returning
@@ -1094,17 +1094,25 @@ static void iommu_poll_ga_log(struct amd_iommu *iommu)
 
 		/* Handle GA entry */
 		switch (GA_REQ_TYPE(log_entry)) {
-		case GA_GUEST_NR:
-			if (!iommu_ga_log_notifier)
-				break;
-
-			pr_debug("%s: devid=%#x, ga_tag=%#x\n",
-				 __func__, GA_DEVID(log_entry),
-				 GA_TAG(log_entry));
+		case GA_GUEST_NR: {
+			int (*notifier)(u32 ga_tag);
+			int ret = 0;
+
+			scoped_guard(rcu) {
+				notifier = rcu_dereference(iommu_ga_log_notifier);
+				if (notifier)
+					ret = notifier(GA_TAG(log_entry));
+			}
 
-			if (iommu_ga_log_notifier(GA_TAG(log_entry)) != 0)
-				pr_err("GA log notifier failed.\n");
+			if (notifier) {
+				pr_debug("%s: devid=%#x, ga_tag=%#x\n",
+					 __func__, GA_DEVID(log_entry),
+					 GA_TAG(log_entry));
+				if (ret)
+					pr_err("GA log notifier failed.\n");
+			}
 			break;
+		}
 		default:
 			break;
 		}
-- 
2.9.4

答复: [PATCH] iommu/amd: RCU-protect iommu_ga_log_notifier

[Li,Rongqing]

> From: Li RongQing <lirongqing@baidu.com>
> 
> iommu_poll_ga_log() accesses iommu_ga_log_notifier without an RCU
> read-side critical section, so synchronize_rcu() in the unregister path does not
> wait for an in-flight notifier callback.
> 
> Use rcu_assign_pointer() when updating the notifier and
> rcu_dereference() under scoped_guard(rcu) when invoking it. Load the
> pointer once to close the race with unregister.
> 
> Keep the RCU read-side critical section minimal: only the pointer load and
> the indirect notifier call stay inside scoped_guard(rcu). Move pr_debug and
> pr_err outside the section so that logging does not extend the window that
> synchronize_rcu() must wait for.
> 

Ping

thanks

[Li,Rongqing] 


> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  drivers/iommu/amd/iommu.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index a06cd61..a31464a 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -1054,11 +1054,11 @@ static void iommu_poll_events(struct
> amd_iommu *iommu)  }
> 
>  #ifdef CONFIG_IRQ_REMAP
> -static int (*iommu_ga_log_notifier)(u32);
> +static int (__rcu *iommu_ga_log_notifier)(u32 ga_tag);
> 
>  int amd_iommu_register_ga_log_notifier(int (*notifier)(u32))  {
> -	iommu_ga_log_notifier = notifier;
> +	rcu_assign_pointer(iommu_ga_log_notifier, notifier);
> 
>  	/*
>  	 * Ensure all in-flight IRQ handlers run to completion before returning
> @@ -1094,17 +1094,25 @@ static void iommu_poll_ga_log(struct
> amd_iommu *iommu)
> 
>  		/* Handle GA entry */
>  		switch (GA_REQ_TYPE(log_entry)) {
> -		case GA_GUEST_NR:
> -			if (!iommu_ga_log_notifier)
> -				break;
> -
> -			pr_debug("%s: devid=%#x, ga_tag=%#x\n",
> -				 __func__, GA_DEVID(log_entry),
> -				 GA_TAG(log_entry));
> +		case GA_GUEST_NR: {
> +			int (*notifier)(u32 ga_tag);
> +			int ret = 0;
> +
> +			scoped_guard(rcu) {
> +				notifier = rcu_dereference(iommu_ga_log_notifier);
> +				if (notifier)
> +					ret = notifier(GA_TAG(log_entry));
> +			}
> 
> -			if (iommu_ga_log_notifier(GA_TAG(log_entry)) != 0)
> -				pr_err("GA log notifier failed.\n");
> +			if (notifier) {
> +				pr_debug("%s: devid=%#x, ga_tag=%#x\n",
> +					 __func__, GA_DEVID(log_entry),
> +					 GA_TAG(log_entry));
> +				if (ret)
> +					pr_err("GA log notifier failed.\n");
> +			}
>  			break;
> +		}
>  		default:
>  			break;
>  		}
> --
> 2.9.4