diff mbox

[-v5,6/5] context_tracking: fix exception_enter when already in IN_KERNEL

Message ID 20150211144319.2a0c29d6@cuia.bos.redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rik van Riel Feb. 11, 2015, 7:43 p.m. UTC
If exception_enter happens when already in IN_KERNEL state, the
code still calls context_tracking_exit, which ends up in
rcu_eqs_exit_common, which explodes with a WARN_ON when it is
called in a situation where dynticks are not enabled.

This can be avoided by having exception_enter only switch to
IN_KERNEL state if the current state is not already IN_KERNEL.

Signed-off-by: Rik van Riel <riel@redhat.com>
Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
---
Frederic, you will want this "bonus" patch, too :)

Thanks to Luiz for finding this one. Whatever I was running did not
trigger this issue...

 include/linux/context_tracking.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Paul E. McKenney Feb. 11, 2015, 9:27 p.m. UTC | #1
On Wed, Feb 11, 2015 at 02:43:19PM -0500, Rik van Riel wrote:
> If exception_enter happens when already in IN_KERNEL state, the
> code still calls context_tracking_exit, which ends up in
> rcu_eqs_exit_common, which explodes with a WARN_ON when it is
> called in a situation where dynticks are not enabled.
> 
> This can be avoided by having exception_enter only switch to
> IN_KERNEL state if the current state is not already IN_KERNEL.

Ugh...  Time to formally verify, sounds like...

							Thanx, Paul

> Signed-off-by: Rik van Riel <riel@redhat.com>
> Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> Frederic, you will want this "bonus" patch, too :)
> 
> Thanks to Luiz for finding this one. Whatever I was running did not
> trigger this issue...
> 
>  include/linux/context_tracking.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
> index b65fd1420e53..9da230406e8c 100644
> --- a/include/linux/context_tracking.h
> +++ b/include/linux/context_tracking.h
> @@ -37,7 +37,8 @@ static inline enum ctx_state exception_enter(void)
>  		return 0;
> 
>  	prev_ctx = this_cpu_read(context_tracking.state);
> -	context_tracking_exit(prev_ctx);
> +	if (prev_ctx != IN_KERNEL)
> +		context_tracking_exit(prev_ctx);
> 
>  	return prev_ctx;
>  }
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frederic Weisbecker Feb. 12, 2015, 3:42 p.m. UTC | #2
On Wed, Feb 11, 2015 at 02:43:19PM -0500, Rik van Riel wrote:
> If exception_enter happens when already in IN_KERNEL state, the
> code still calls context_tracking_exit, which ends up in
> rcu_eqs_exit_common, which explodes with a WARN_ON when it is
> called in a situation where dynticks are not enabled.

Fortunately context_tracking_exit() already has a current_state == IN_KERNEL
check so this shouldn't be a problem.

Meanwhile I'll still take the patch, it's better to handle that
from the caller.

Thanks.

> 
> This can be avoided by having exception_enter only switch to
> IN_KERNEL state if the current state is not already IN_KERNEL.
> 
> Signed-off-by: Rik van Riel <riel@redhat.com>
> Reported-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> Frederic, you will want this "bonus" patch, too :)
> 
> Thanks to Luiz for finding this one. Whatever I was running did not
> trigger this issue...
> 
>  include/linux/context_tracking.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
> index b65fd1420e53..9da230406e8c 100644
> --- a/include/linux/context_tracking.h
> +++ b/include/linux/context_tracking.h
> @@ -37,7 +37,8 @@ static inline enum ctx_state exception_enter(void)
>  		return 0;
>  
>  	prev_ctx = this_cpu_read(context_tracking.state);
> -	context_tracking_exit(prev_ctx);
> +	if (prev_ctx != IN_KERNEL)
> +		context_tracking_exit(prev_ctx);
>  
>  	return prev_ctx;
>  }
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rik van Riel Feb. 12, 2015, 3:47 p.m. UTC | #3
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/12/2015 10:42 AM, Frederic Weisbecker wrote:
> On Wed, Feb 11, 2015 at 02:43:19PM -0500, Rik van Riel wrote:
>> If exception_enter happens when already in IN_KERNEL state, the 
>> code still calls context_tracking_exit, which ends up in 
>> rcu_eqs_exit_common, which explodes with a WARN_ON when it is 
>> called in a situation where dynticks are not enabled.
> 
> Fortunately context_tracking_exit() already has a current_state ==
> IN_KERNEL check so this shouldn't be a problem.

No, it had a hard-coded "current_state == IN_USER" check,
which is very close, but ...

... I replaced that with a state argument, and forgot to
ensure that it never gets called with state == IN_KERNEL.
This patch fixes that.

> Meanwhile I'll still take the patch, it's better to handle that 
> from the caller.

Thanks.

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU3Mr+AAoJEM553pKExN6DYNUH/2m9CtXhLdTHOEHRvxg41PCZ
/xafetUOS9cka0CNuiYpUuvfMSucoePW7YqUXqjYSIP25DsAleOh0qdep1Ob5bH+
2BqZNMwK3QDHf1+/V7nulnjVkeHtpXJm0HIZOjc06xeL+9T6ydB1vhQGIMLrGL9S
LvOstI3fseeIgglwYc2Gx7H7e99oOkxysvwMMvcMrW0cPSRAOdYxINQnfYW8A5kq
DTTXwWuJRZa4FLtP3wLpvocm5dMGDwTsDmuOk1PmXYlsTsO6H2BmCeio0euzStoJ
l+jR4x7Aq2KXES7gnMgpPw1iON3xKJ/RbXF8IC/doII8FYEV8Raxnf7hl47etBw=
=yIjW
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frederic Weisbecker Feb. 12, 2015, 5 p.m. UTC | #4
On Thu, Feb 12, 2015 at 10:47:10AM -0500, Rik van Riel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/12/2015 10:42 AM, Frederic Weisbecker wrote:
> > On Wed, Feb 11, 2015 at 02:43:19PM -0500, Rik van Riel wrote:
> >> If exception_enter happens when already in IN_KERNEL state, the 
> >> code still calls context_tracking_exit, which ends up in 
> >> rcu_eqs_exit_common, which explodes with a WARN_ON when it is 
> >> called in a situation where dynticks are not enabled.
> > 
> > Fortunately context_tracking_exit() already has a current_state ==
> > IN_KERNEL check so this shouldn't be a problem.
> 
> No, it had a hard-coded "current_state == IN_USER" check,
> which is very close, but ...
> 
> ... I replaced that with a state argument, and forgot to
> ensure that it never gets called with state == IN_KERNEL.
> This patch fixes that.

Ah that's right! Well I'm going to merge this patch to 1/5 then to
avoid breaking bisection.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rik van Riel Feb. 12, 2015, 5:48 p.m. UTC | #5
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/12/2015 12:00 PM, Frederic Weisbecker wrote:
> On Thu, Feb 12, 2015 at 10:47:10AM -0500, Rik van Riel wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 02/12/2015 10:42 AM, Frederic Weisbecker wrote:
>>> On Wed, Feb 11, 2015 at 02:43:19PM -0500, Rik van Riel wrote:
>>>> If exception_enter happens when already in IN_KERNEL state,
>>>> the code still calls context_tracking_exit, which ends up in
>>>>  rcu_eqs_exit_common, which explodes with a WARN_ON when it
>>>> is called in a situation where dynticks are not enabled.
>>> 
>>> Fortunately context_tracking_exit() already has a current_state
>>> == IN_KERNEL check so this shouldn't be a problem.
>> 
>> No, it had a hard-coded "current_state == IN_USER" check, which
>> is very close, but ...
>> 
>> ... I replaced that with a state argument, and forgot to ensure
>> that it never gets called with state == IN_KERNEL. This patch
>> fixes that.
> 
> Ah that's right! Well I'm going to merge this patch to 1/5 then to 
> avoid breaking bisection.

Thank you, Frederic!

- -- 
All rights reversed
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJU3OeDAAoJEM553pKExN6D7BsIAJ8CKC73jQ8T5Dqa/tlHV7Db
QFSJdpxP+7jCZwssehgpjpxCwtJ0UvGgle5OwX/POUhmagHxHmxVydOBz+xfYdBr
UuGkEl5TL+oyoMUr80Q4RTnJSZN08zi+THqiv33tyPUj6cNiycBZAuho3ELTRNOA
bRcHrMW+xd95uqoung7dSKrgA2jcym3+umNGnQb0gniraqcNLAmWs+jfAO8yZLJg
vk8bIKed6epQ3n6gcdYe0A28cLOuBvjEs5JNcEPxujY/349sjitKR2pLQ6HsfHLV
frlKsh7qQIRtoUJLO9ZBBDtGrmThwBwH8rw+GcVR8zviPNvV4IRrx47VBcHDWjc=
=mwFO
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index b65fd1420e53..9da230406e8c 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -37,7 +37,8 @@  static inline enum ctx_state exception_enter(void)
 		return 0;
 
 	prev_ctx = this_cpu_read(context_tracking.state);
-	context_tracking_exit(prev_ctx);
+	if (prev_ctx != IN_KERNEL)
+		context_tracking_exit(prev_ctx);
 
 	return prev_ctx;
 }