[PATCH] powerpc/xmon: Fix invocation inside lock region

[PATCH] powerpc/xmon: Fix invocation inside lock region

[Breno Leitao]

Currently xmon needs to get devtree_lock (through rtas_token()) during its
invocation (at crash time). If there is a crash while devtree_lock is being
held, then xmon tries to get the lock but spins forever and never get into
the interactive debugger, as in the following case:

	int *ptr = NULL;
	raw_spin_lock_irqsave(&devtree_lock, flags);
	*ptr = 0xdeadbeef;

This patch avoids calling rtas_token(), thus trying to get the same lock,
at crash time. This new mechanism proposes getting the token at
initialization time (xmon_init()) and just consuming it at crash time.

This would allow xmon to be possible invoked independent of devtree_lock
being held or not.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/xmon/xmon.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 36b8dc47a3c3..b566203d09c5 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -75,6 +75,9 @@ static int xmon_gate;
 #define xmon_owner 0
 #endif /* CONFIG_SMP */
 
+#ifdef CONFIG_PPC_PSERIES
+static int set_indicator_token = RTAS_UNKNOWN_SERVICE;
+#endif
 static unsigned long in_xmon __read_mostly = 0;
 static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT);
 
@@ -358,7 +361,6 @@ static inline void disable_surveillance(void)
 #ifdef CONFIG_PPC_PSERIES
 	/* Since this can't be a module, args should end up below 4GB. */
 	static struct rtas_args args;
-	int token;
 
 	/*
 	 * At this point we have got all the cpus we can into
@@ -367,11 +369,11 @@ static inline void disable_surveillance(void)
 	 * If we did try to take rtas.lock there would be a
 	 * real possibility of deadlock.
 	 */
-	token = rtas_token("set-indicator");
-	if (token == RTAS_UNKNOWN_SERVICE)
+	if (set_indicator_token == RTAS_UNKNOWN_SERVICE)
 		return;
 
-	rtas_call_unlocked(&args, token, 3, 1, NULL, SURVEILLANCE_TOKEN, 0, 0);
+	rtas_call_unlocked(&args, set_indicator_token, 3, 1, NULL,
+			   SURVEILLANCE_TOKEN, 0, 0);
 
 #endif /* CONFIG_PPC_PSERIES */
 }
@@ -3688,6 +3690,14 @@ static void xmon_init(int enable)
 		__debugger_iabr_match = xmon_iabr_match;
 		__debugger_break_match = xmon_break_match;
 		__debugger_fault_handler = xmon_fault_handler;
+
+#ifdef CONFIG_PPC_PSERIES
+		/*
+		 * Get the token here to avoid trying to get a lock
+		 * during the crash, causing a deadlock.
+		 */
+		set_indicator_token = rtas_token("set-indicator");
+#endif
 	} else {
 		__debugger = NULL;
 		__debugger_ipi = NULL;
-- 
2.19.0

Re: [PATCH] powerpc/xmon: Fix invocation inside lock region

[Thiago Jung Bauermann]

Breno Leitao <leitao@debian.org> writes:

> Currently xmon needs to get devtree_lock (through rtas_token()) during its
> invocation (at crash time). If there is a crash while devtree_lock is being
> held, then xmon tries to get the lock but spins forever and never get into
> the interactive debugger, as in the following case:
>
> 	int *ptr = NULL;
> 	raw_spin_lock_irqsave(&devtree_lock, flags);
> 	*ptr = 0xdeadbeef;
>
> This patch avoids calling rtas_token(), thus trying to get the same lock,
> at crash time. This new mechanism proposes getting the token at
> initialization time (xmon_init()) and just consuming it at crash time.
>
> This would allow xmon to be possible invoked independent of devtree_lock
> being held or not.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>

I'm not familiar with xmon code but I was curious, and for what it's
worth:

Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

> ---
>  arch/powerpc/xmon/xmon.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 36b8dc47a3c3..b566203d09c5 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -75,6 +75,9 @@ static int xmon_gate;
>  #define xmon_owner 0
>  #endif /* CONFIG_SMP */
>
> +#ifdef CONFIG_PPC_PSERIES
> +static int set_indicator_token = RTAS_UNKNOWN_SERVICE;
> +#endif
>  static unsigned long in_xmon __read_mostly = 0;
>  static int xmon_on = IS_ENABLED(CONFIG_XMON_DEFAULT);
>
> @@ -358,7 +361,6 @@ static inline void disable_surveillance(void)
>  #ifdef CONFIG_PPC_PSERIES
>  	/* Since this can't be a module, args should end up below 4GB. */
>  	static struct rtas_args args;
> -	int token;
>
>  	/*
>  	 * At this point we have got all the cpus we can into
> @@ -367,11 +369,11 @@ static inline void disable_surveillance(void)
>  	 * If we did try to take rtas.lock there would be a
>  	 * real possibility of deadlock.
>  	 */
> -	token = rtas_token("set-indicator");
> -	if (token == RTAS_UNKNOWN_SERVICE)
> +	if (set_indicator_token == RTAS_UNKNOWN_SERVICE)
>  		return;
>
> -	rtas_call_unlocked(&args, token, 3, 1, NULL, SURVEILLANCE_TOKEN, 0, 0);
> +	rtas_call_unlocked(&args, set_indicator_token, 3, 1, NULL,
> +			   SURVEILLANCE_TOKEN, 0, 0);
>
>  #endif /* CONFIG_PPC_PSERIES */
>  }
> @@ -3688,6 +3690,14 @@ static void xmon_init(int enable)
>  		__debugger_iabr_match = xmon_iabr_match;
>  		__debugger_break_match = xmon_break_match;
>  		__debugger_fault_handler = xmon_fault_handler;
> +
> +#ifdef CONFIG_PPC_PSERIES
> +		/*
> +		 * Get the token here to avoid trying to get a lock
> +		 * during the crash, causing a deadlock.
> +		 */
> +		set_indicator_token = rtas_token("set-indicator");
> +#endif
>  	} else {
>  		__debugger = NULL;
>  		__debugger_ipi = NULL;


--
Thiago Jung Bauermann
IBM Linux Technology Center

Re: powerpc/xmon: Fix invocation inside lock region

[Michael Ellerman]

On Thu, 2018-11-08 at 17:12:42 UTC, Breno Leitao wrote:
> Currently xmon needs to get devtree_lock (through rtas_token()) during its
> invocation (at crash time). If there is a crash while devtree_lock is being
> held, then xmon tries to get the lock but spins forever and never get into
> the interactive debugger, as in the following case:
> 
> 	int *ptr = NULL;
> 	raw_spin_lock_irqsave(&devtree_lock, flags);
> 	*ptr = 0xdeadbeef;
> 
> This patch avoids calling rtas_token(), thus trying to get the same lock,
> at crash time. This new mechanism proposes getting the token at
> initialization time (xmon_init()) and just consuming it at crash time.
> 
> This would allow xmon to be possible invoked independent of devtree_lock
> being held or not.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/8d4a862276a9c30a269d368d324fb5

cheers