[PATCH] mce: fix RCU lockdep from mce_log()

[PATCH] mce: fix RCU lockdep from mce_log()

[Davidlohr Bueso]

Hi,

Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
This does "fix" the message though and I can use MCE normally.

Thanks,
Davidlohr


From: Davidlohr Bueso <dave@gnu.org>

Based on the following message:

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!

other info that might help us debug this:

rcu_scheduler_active = 1, debug_locks = 1
no locks held by mcelog/2350.

stack backtrace:
Pid: 2350, comm: mcelog Tainted: G        W   2.6.37-rc1+ #7
Call Trace:
 [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
 [<ffffffff810189e9>] mce_poll+0xa9/0xd0
 [<ffffffff81160585>] do_sys_poll+0x275/0x550
 [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
 [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
 [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
 [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
 [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
 [<ffffffff81121f08>] ? __do_fault+0x128/0x470
 [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
 [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
 [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
 [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
 [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
 [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
 [<ffffffff811608a7>] sys_ppoll+0x47/0x190
 [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
 [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b

At this point the arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!

other info that might help us debug this:

rcu_scheduler_active = 1, debug_locks = 1
no locks held by mcelog/2350.

stack backtrace:
Pid: 2350, comm: mcelog Tainted: G        W   2.6.37-rc1+ #7
Call Trace:
 [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
 [<ffffffff810189e9>] mce_poll+0xa9/0xd0
 [<ffffffff81160585>] do_sys_poll+0x275/0x550
 [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
 [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
 [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
 [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
 [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
 [<ffffffff81121f08>] ? __do_fault+0x128/0x470
 [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
 [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
 [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
 [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
 [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
 [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
 [<ffffffff811608a7>] sys_ppoll+0x47/0x190
 [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
 [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b

At this point the lockdep_is_held(&mce_read_mutex) call is failing.
So check if the mce_read_mutex is held before derefencing instead of using rcu_dereference_check_mce()

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 arch/x86/kernel/cpu/mcheck/mce.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 7a35b72..6f95b2c 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1625,8 +1625,12 @@ out:
 static unsigned int mce_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &mce_wait, wait);
-	if (rcu_dereference_check_mce(mcelog.next))
-		return POLLIN | POLLRDNORM;
+
+	if (mutex_is_locked(&mce_read_mutex)) {
+		if (rcu_dereference_index_check(mcelog.next, 
+						rcu_read_lock_sched_held()))
+			return POLLIN | POLLRDNORM;
+	}
 	if (!mce_apei_read_done && apei_check_mce())
 		return POLLIN | POLLRDNORM;
 	return 0;
-- 
1.7.1

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Davidlohr Bueso]

Sorry, the title should have been:
[PATCH] mce: fix RCU lockdep from mce_poll()

On Fri, 2010-11-05 at 18:44 -0300, Davidlohr Bueso wrote:
> Hi,
> 
> Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> This does "fix" the message though and I can use MCE normally.
> 
> Thanks,
> Davidlohr
> 
> 
> From: Davidlohr Bueso <dave@gnu.org>
> 
> Based on the following message:
> 
> ===================================================
> [ INFO: suspicious rcu_dereference_check() usage. ]
> ---------------------------------------------------
> arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!
> 
> other info that might help us debug this:
> 
> rcu_scheduler_active = 1, debug_locks = 1
> no locks held by mcelog/2350.
> 
> stack backtrace:
> Pid: 2350, comm: mcelog Tainted: G        W   2.6.37-rc1+ #7
> Call Trace:
>  [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
>  [<ffffffff810189e9>] mce_poll+0xa9/0xd0
>  [<ffffffff81160585>] do_sys_poll+0x275/0x550
>  [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
>  [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
>  [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
>  [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
>  [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
>  [<ffffffff81121f08>] ? __do_fault+0x128/0x470
>  [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
>  [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
>  [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
>  [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
>  [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
>  [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
>  [<ffffffff811608a7>] sys_ppoll+0x47/0x190
>  [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
>  [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>  [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
> 
> At this point the arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!
> 
> other info that might help us debug this:
> 
> rcu_scheduler_active = 1, debug_locks = 1
> no locks held by mcelog/2350.
> 
> stack backtrace:
> Pid: 2350, comm: mcelog Tainted: G        W   2.6.37-rc1+ #7
> Call Trace:
>  [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
>  [<ffffffff810189e9>] mce_poll+0xa9/0xd0
>  [<ffffffff81160585>] do_sys_poll+0x275/0x550
>  [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
>  [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
>  [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
>  [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
>  [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
>  [<ffffffff81121f08>] ? __do_fault+0x128/0x470
>  [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
>  [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
>  [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
>  [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
>  [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
>  [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
>  [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
>  [<ffffffff811608a7>] sys_ppoll+0x47/0x190
>  [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
>  [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>  [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
> 
> At this point the lockdep_is_held(&mce_read_mutex) call is failing.
> So check if the mce_read_mutex is held before derefencing instead of using rcu_dereference_check_mce()
> 
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 7a35b72..6f95b2c 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1625,8 +1625,12 @@ out:
>  static unsigned int mce_poll(struct file *file, poll_table *wait)
>  {
>  	poll_wait(file, &mce_wait, wait);
> -	if (rcu_dereference_check_mce(mcelog.next))
> -		return POLLIN | POLLRDNORM;
> +
> +	if (mutex_is_locked(&mce_read_mutex)) {
> +		if (rcu_dereference_index_check(mcelog.next, 
> +						rcu_read_lock_sched_held()))
> +			return POLLIN | POLLRDNORM;
> +	}
>  	if (!mce_apei_read_done && apei_check_mce())
>  		return POLLIN | POLLRDNORM;
>  	return 0;
> -- 
> 1.7.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Andi Kleen]

On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> Hi,
> 
> Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> This does "fix" the message though and I can use MCE normally.

The patch is certainly not correct. The variable needs to be read
independently of the mutex.

-Andi

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Paul E. McKenney]

On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > Hi,
> > 
> > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > This does "fix" the message though and I can use MCE normally.
> 
> The patch is certainly not correct. The variable needs to be read
> independently of the mutex.

This code is simply checking the value of the pointer, and therefore
need not protect any actual dereferences.  So why not replace the
rcu_dereference_check_mce() with rcu_access_pointer()?  If this is
OK, please see the patch below.

BTW, assigning the value returned by rcu_access_pointer() into a
variable often indicates a bug.  ;-)

							Thanx, Paul

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 7a35b72..4d29d50 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1625,7 +1625,7 @@ out:
 static unsigned int mce_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &mce_wait, wait);
-	if (rcu_dereference_check_mce(mcelog.next))
+	if (rcu_access_pointer(mcelog.next))
 		return POLLIN | POLLRDNORM;
 	if (!mce_apei_read_done && apei_check_mce())
 		return POLLIN | POLLRDNORM;

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Davidlohr Bueso]

On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > > Hi,
> > > 
> > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > > This does "fix" the message though and I can use MCE normally.
> > 
> > The patch is certainly not correct. The variable needs to be read
> > independently of the mutex.
> 
> This code is simply checking the value of the pointer, and therefore
> need not protect any actual dereferences.  So why not replace the
> rcu_dereference_check_mce() with rcu_access_pointer()?  If this is
> OK, please see the patch below.
> 
> BTW, assigning the value returned by rcu_access_pointer() into a
> variable often indicates a bug.  ;-)
> 
> 							Thanx, Paul
> 
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 7a35b72..4d29d50 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1625,7 +1625,7 @@ out:
>  static unsigned int mce_poll(struct file *file, poll_table *wait)
>  {
>  	poll_wait(file, &mce_wait, wait);
> -	if (rcu_dereference_check_mce(mcelog.next))
> +	if (rcu_access_pointer(mcelog.next))

this doesn't compile (mcelog.next is an index):

arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
‘unary *’ (have ‘unsigned int’)
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
in declaration of ‘_________p1’
arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
‘unary *’ (have ‘unsigned int’)
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
in declaration of ‘type name’
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
integer of different size
arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
‘unary *’ (have ‘unsigned int’)
arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
in declaration of ‘type name’
make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1


Since the mutex is independent, what about this patch?

 Signed-off-by: Davidlohr Bueso <dave@gnu.org>

---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c
b/arch/x86/kernel/cpu/mcheck/mce.c
index 7a35b72..cc1c673 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1625,7 +1625,7 @@ out:
 static unsigned int mce_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &mce_wait, wait);
-	if (rcu_dereference_check_mce(mcelog.next))
+	if (rcu_dereference_index_check(mcelog.next,
rcu_read_lock_sched_held()))
 		return POLLIN | POLLRDNORM;
 	if (!mce_apei_read_done && apei_check_mce())
 		return POLLIN | POLLRDNORM;
-- 
1.7.1

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Paul E. McKenney]

On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
> On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > > > Hi,
> > > > 
> > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > > > This does "fix" the message though and I can use MCE normally.
> > > 
> > > The patch is certainly not correct. The variable needs to be read
> > > independently of the mutex.
> > 
> > This code is simply checking the value of the pointer, and therefore
> > need not protect any actual dereferences.  So why not replace the
> > rcu_dereference_check_mce() with rcu_access_pointer()?  If this is
> > OK, please see the patch below.
> > 
> > BTW, assigning the value returned by rcu_access_pointer() into a
> > variable often indicates a bug.  ;-)
> > 
> > 							Thanx, Paul
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > index 7a35b72..4d29d50 100644
> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > @@ -1625,7 +1625,7 @@ out:
> >  static unsigned int mce_poll(struct file *file, poll_table *wait)
> >  {
> >  	poll_wait(file, &mce_wait, wait);
> > -	if (rcu_dereference_check_mce(mcelog.next))
> > +	if (rcu_access_pointer(mcelog.next))
> 
> this doesn't compile (mcelog.next is an index):
> 
> arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> ‘unary *’ (have ‘unsigned int’)
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> in declaration of ‘_________p1’
> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> ‘unary *’ (have ‘unsigned int’)
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> in declaration of ‘type name’
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
> integer of different size
> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> ‘unary *’ (have ‘unsigned int’)
> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> in declaration of ‘type name’
> make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
> 
> 
> Since the mutex is independent, what about this patch?

Looks good to me!

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

>  Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> 
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c
> b/arch/x86/kernel/cpu/mcheck/mce.c
> index 7a35b72..cc1c673 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1625,7 +1625,7 @@ out:
>  static unsigned int mce_poll(struct file *file, poll_table *wait)
>  {
>  	poll_wait(file, &mce_wait, wait);
> -	if (rcu_dereference_check_mce(mcelog.next))
> +	if (rcu_dereference_index_check(mcelog.next,
> rcu_read_lock_sched_held()))
>  		return POLLIN | POLLRDNORM;
>  	if (!mce_apei_read_done && apei_check_mce())
>  		return POLLIN | POLLRDNORM;
> -- 
> 1.7.1
> 
> 
> 

Re: [PATCH] mce: fix RCU lockdep from mce_poll()

[Davidlohr Bueso]

On Mon, 2010-11-08 at 05:17 -0800, Paul E. McKenney wrote:
> On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
> > On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> > > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> > > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> > > > > Hi,
> > > > > 
> > > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> > > > > This does "fix" the message though and I can use MCE normally.
> > > > 
> > > > The patch is certainly not correct. The variable needs to be read
> > > > independently of the mutex.
> > > 
> > > This code is simply checking the value of the pointer, and therefore
> > > need not protect any actual dereferences.  So why not replace the
> > > rcu_dereference_check_mce() with rcu_access_pointer()?  If this is
> > > OK, please see the patch below.
> > > 
> > > BTW, assigning the value returned by rcu_access_pointer() into a
> > > variable often indicates a bug.  ;-)
> > > 
> > > 							Thanx, Paul
> > > 
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > 
> > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> > > index 7a35b72..4d29d50 100644
> > > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> > > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> > > @@ -1625,7 +1625,7 @@ out:
> > >  static unsigned int mce_poll(struct file *file, poll_table *wait)
> > >  {
> > >  	poll_wait(file, &mce_wait, wait);
> > > -	if (rcu_dereference_check_mce(mcelog.next))
> > > +	if (rcu_access_pointer(mcelog.next))
> > 
> > this doesn't compile (mcelog.next is an index):
> > 
> > arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> > ‘unary *’ (have ‘unsigned int’)
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> > in declaration of ‘_________p1’
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> > ‘unary *’ (have ‘unsigned int’)
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> > in declaration of ‘type name’
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
> > integer of different size
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> > ‘unary *’ (have ‘unsigned int’)
> > arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> > in declaration of ‘type name’
> > make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
> > 
> > 
> > Since the mutex is independent, what about this patch?
> 
> Looks good to me!
> 
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 

Andi, any further thoughts?

Thanks,
Davidlohr

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Zdenek Kabelac]

2010/11/8 Paul E. McKenney <paulmck@linux.vnet.ibm.com>:
> On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
>> On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
>> > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
>> > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
>> > > > Hi,
>> > > >
>> > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
>> > > > This does "fix" the message though and I can use MCE normally.
>> > >
>> > > The patch is certainly not correct. The variable needs to be read
>> > > independently of the mutex.
>> >
>> > This code is simply checking the value of the pointer, and therefore
>> > need not protect any actual dereferences.  So why not replace the
>> > rcu_dereference_check_mce() with rcu_access_pointer()?  If this is
>> > OK, please see the patch below.
>> >
>> > BTW, assigning the value returned by rcu_access_pointer() into a
>> > variable often indicates a bug.  ;-)
>> >
>> >                                                     Thanx, Paul
>> >
>> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> >
>> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> > index 7a35b72..4d29d50 100644
>> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> > @@ -1625,7 +1625,7 @@ out:
>> >  static unsigned int mce_poll(struct file *file, poll_table *wait)
>> >  {
>> >     poll_wait(file, &mce_wait, wait);
>> > -   if (rcu_dereference_check_mce(mcelog.next))
>> > +   if (rcu_access_pointer(mcelog.next))
>>
>> this doesn't compile (mcelog.next is an index):
>>
>> arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
>> ‘unary *’ (have ‘unsigned int’)
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
>> in declaration of ‘_________p1’
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
>> ‘unary *’ (have ‘unsigned int’)
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
>> in declaration of ‘type name’
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
>> integer of different size
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
>> ‘unary *’ (have ‘unsigned int’)
>> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
>> in declaration of ‘type name’
>> make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
>>
>>
>> Since the mutex is independent, what about this patch?
>
> Looks good to me!
>
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
>>  Signed-off-by: Davidlohr Bueso <dave@gnu.org>
>>
>> ---
>>  arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c
>> b/arch/x86/kernel/cpu/mcheck/mce.c
>> index 7a35b72..cc1c673 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -1625,7 +1625,7 @@ out:
>>  static unsigned int mce_poll(struct file *file, poll_table *wait)
>>  {
>>       poll_wait(file, &mce_wait, wait);
>> -     if (rcu_dereference_check_mce(mcelog.next))
>> +     if (rcu_dereference_index_check(mcelog.next,
>> rcu_read_lock_sched_held()))
>>               return POLLIN | POLLRDNORM;
>>       if (!mce_apei_read_done && apei_check_mce())
>>               return POLLIN | POLLRDNORM;



Any chance to have this ever fixed upstream ?
(still happens with today's vanialla build)

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
arch/x86/kernel/cpu/mcheck/mce.c:1629 invoked rcu_dereference_check()
without protection!

other info that might help us debug this:


rcu_scheduler_active = 1, debug_locks = 0
no locks held by mcelog/772.

stack backtrace:
Pid: 772, comm: mcelog Not tainted 2.6.38-09069-gc7dfeb9 #103
Call Trace:
 [<ffffffff8108bb7b>] lockdep_rcu_dereference+0xbb/0xc0
 [<ffffffff81017c55>] mce_poll+0xa5/0xd0
 [<ffffffff81161be0>] do_sys_poll+0x270/0x500
 [<ffffffff81160780>] ? poll_freewait+0xe0/0xe0
 [<ffffffff81160870>] ? __pollwait+0xf0/0xf0
 [<ffffffff81160870>] ? __pollwait+0xf0/0xf0
 [<ffffffff8111d678>] ? __do_fault+0x128/0x490
 [<ffffffff81009136>] ? native_sched_clock+0x26/0x70
 [<ffffffff8107d9b7>] ? local_clock+0x47/0x60
 [<ffffffff8108a558>] ? trace_hardirqs_off_caller+0x28/0xc0
 [<ffffffff8108dc00>] ? __lock_acquire+0x410/0x1bb0
 [<ffffffff81120284>] ? handle_pte_fault+0x84/0x900
 [<ffffffff811049dd>] ? __free_pages+0x2d/0x40
 [<ffffffff8111e0c0>] ? __pte_alloc+0xd0/0x120
 [<ffffffff81064a81>] ? sigprocmask+0x41/0x100
 [<ffffffff81498969>] ? sub_preempt_count+0xa9/0xe0
 [<ffffffff8149528b>] ? _raw_spin_unlock_irq+0x3b/0x60
 [<ffffffff81064a0b>] ? recalc_sigpending+0x1b/0x50
 [<ffffffff811620d4>] sys_ppoll+0xe4/0x180
 [<ffffffff812946ce>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff8149c86b>] system_call_fastpath+0x16/0x1b


Zdenek

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Davidlohr Bueso]

On Tue, 2011-03-29 at 11:45 +0200, Zdenek Kabelac wrote:
> 2010/11/8 Paul E. McKenney <paulmck@linux.vnet.ibm.com>:
> > On Mon, Nov 08, 2010 at 08:30:19AM -0300, Davidlohr Bueso wrote:
> >> On Sun, 2010-11-07 at 05:39 -0800, Paul E. McKenney wrote:
> >> > On Sat, Nov 06, 2010 at 07:53:50PM +0100, Andi Kleen wrote:
> >> > > On Fri, Nov 05, 2010 at 06:44:59PM -0300, Davidlohr Bueso wrote:
> >> > > > Hi,
> >> > > >
> >> > > > Please review this patch, I am not very familiar with MCE/RCU so I'm not sure that this is the correct fix (otherwise consider it a bug report :)).
> >> > > > This does "fix" the message though and I can use MCE normally.
> >> > >
> >> > > The patch is certainly not correct. The variable needs to be read
> >> > > independently of the mutex.
> >> >
> >> > This code is simply checking the value of the pointer, and therefore
> >> > need not protect any actual dereferences.  So why not replace the
> >> > rcu_dereference_check_mce() with rcu_access_pointer()?  If this is
> >> > OK, please see the patch below.
> >> >
> >> > BTW, assigning the value returned by rcu_access_pointer() into a
> >> > variable often indicates a bug.  ;-)
> >> >
> >> >                                                     Thanx, Paul
> >> >
> >> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >> >
> >> > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> >> > index 7a35b72..4d29d50 100644
> >> > --- a/arch/x86/kernel/cpu/mcheck/mce.c
> >> > +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> >> > @@ -1625,7 +1625,7 @@ out:
> >> >  static unsigned int mce_poll(struct file *file, poll_table *wait)
> >> >  {
> >> >     poll_wait(file, &mce_wait, wait);
> >> > -   if (rcu_dereference_check_mce(mcelog.next))
> >> > +   if (rcu_access_pointer(mcelog.next))
> >>
> >> this doesn't compile (mcelog.next is an index):
> >>
> >> arch/x86/kernel/cpu/mcheck/mce.c: In function ‘mce_poll’:
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> >> ‘unary *’ (have ‘unsigned int’)
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> >> in declaration of ‘_________p1’
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> >> ‘unary *’ (have ‘unsigned int’)
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> >> in declaration of ‘type name’
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: cast to pointer from
> >> integer of different size
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: error: invalid type argument of
> >> ‘unary *’ (have ‘unsigned int’)
> >> arch/x86/kernel/cpu/mcheck/mce.c:1628: warning: type defaults to ‘int’
> >> in declaration of ‘type name’
> >> make[4]: *** [arch/x86/kernel/cpu/mcheck/mce.o] Error 1
> >>
> >>
> >> Since the mutex is independent, what about this patch?
> >
> > Looks good to me!
> >
> > Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> >
> >>  Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> >>
> >> ---
> >>  arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c
> >> b/arch/x86/kernel/cpu/mcheck/mce.c
> >> index 7a35b72..cc1c673 100644
> >> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> >> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> >> @@ -1625,7 +1625,7 @@ out:
> >>  static unsigned int mce_poll(struct file *file, poll_table *wait)
> >>  {
> >>       poll_wait(file, &mce_wait, wait);
> >> -     if (rcu_dereference_check_mce(mcelog.next))
> >> +     if (rcu_dereference_index_check(mcelog.next,
> >> rcu_read_lock_sched_held()))
> >>               return POLLIN | POLLRDNORM;
> >>       if (!mce_apei_read_done && apei_check_mce())
> >>               return POLLIN | POLLRDNORM;
> 
> 
> 
> Any chance to have this ever fixed upstream ?
> (still happens with today's vanialla build)

I'm still quite interested in getting this fixed, I run into it several
times a day.

Re: [PATCH] mce: fix RCU lockdep from mce_log()

[Andi Kleen]

> I'm still quite interested in getting this fixed, I run into it several
> times a day.

Paul needs to send his fix to the x86 maintainers. They were not cc'ed
so never saw this.

-Andi

[PATCH] mce: fix RCU lockdep from mce_poll()

[Davidlohr Bueso]

Based on the following message

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked rcu_dereference_check() without protection!

other info that might help us debug this:

rcu_scheduler_active = 1, debug_locks = 1
no locks held by mcelog/2350.

stack backtrace:
Pid: 2350, comm: mcelog Tainted: G        W   2.6.37-rc1+ #7
Call Trace:
 [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
 [<ffffffff810189e9>] mce_poll+0xa9/0xd0
 [<ffffffff81160585>] do_sys_poll+0x275/0x550
 [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
 [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
 [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
 [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
 [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
 [<ffffffff81121f08>] ? __do_fault+0x128/0x470
 [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
 [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
 [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
 [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
 [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
 [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
 [<ffffffff811608a7>] sys_ppoll+0x47/0x190
 [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
 [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b

This code is simply checking the value of the pointer, and therefore need not protect any actual dereferences.
Replace rcu_dereference_check_mce() with rcu_dereference_index_check().

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
This patch reflects the summary of the activity in https://lkml.org/lkml/2010/11/5/236

 arch/x86/kernel/cpu/mcheck/mce.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 5a05ef6..6cad5dc 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1626,7 +1626,8 @@ out:
 static unsigned int mce_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &mce_wait, wait);
-	if (rcu_dereference_check_mce(mcelog.next))
+	if (rcu_dereference_index_check(mcelog.next,
+					rcu_read_lock_sched_held()))
 		return POLLIN | POLLRDNORM;
 	if (!mce_apei_read_done && apei_check_mce())
 		return POLLIN | POLLRDNORM;
-- 
1.7.1

[tip:x86/urgent] x86, mce: Fix RCU lockdep from mce_poll()

[tip-bot for Davidlohr Bueso]

Commit-ID:  706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
Gitweb:     http://git.kernel.org/tip/706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
Author:     Davidlohr Bueso <dave@gnu.org>
AuthorDate: Wed, 30 Mar 2011 23:13:24 -0300
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 31 Mar 2011 11:06:08 +0200

x86, mce: Fix RCU lockdep from mce_poll()

Based on the following message:

  ===================================================
  [ INFO: suspicious rcu_dereference_check() usage. ]
  ---------------------------------------------------
  arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked
  rcu_dereference_check() without protection!

  other info that might help us debug this:

  rcu_scheduler_active = 1, debug_locks = 1
  no locks held by mcelog/2350.

  stack backtrace:
  Pid: 2350, comm: mcelog Tainted: G        W   2.6.37-rc1+ #7
  Call Trace:
   [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
   [<ffffffff810189e9>] mce_poll+0xa9/0xd0
   [<ffffffff81160585>] do_sys_poll+0x275/0x550
   [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
   [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
   [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
   [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
   [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
   [<ffffffff81121f08>] ? __do_fault+0x128/0x470
   [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
   [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
   [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
   [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
   [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
   [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
   [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
   [<ffffffff811608a7>] sys_ppoll+0x47/0x190
   [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
   [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
   [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b

This code is simply checking the value of the pointer, and
therefore need not protect any actual dereferences. Replace
rcu_dereference_check_mce() with rcu_dereference_index_check().

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Zdenek Kabelac <zdenek.kabelac@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
LKML-Reference: <1301537604.2140.21.camel@offworld>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 5a05ef6..a2d664f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1626,7 +1626,7 @@ out:
 static unsigned int mce_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &mce_wait, wait);
-	if (rcu_dereference_check_mce(mcelog.next))
+	if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
 		return POLLIN | POLLRDNORM;
 	if (!mce_apei_read_done && apei_check_mce())
 		return POLLIN | POLLRDNORM;

Re: [tip:x86/urgent] x86, mce: Fix RCU lockdep from mce_poll()

[Zdenek Kabelac]

However on my machine - I do not a see a difference ?

I've this patch applied - and it still gives me the same error.
(using 6aba74f2791287ec407e0f92487a725a25908067 and this patch)

Here are my RCU config options:

# grep RCU .config
# RCU Subsystem
CONFIG_TREE_PREEMPT_RCU=y
CONFIG_PREEMPT_RCU=y
CONFIG_RCU_TRACE=y
CONFIG_RCU_FANOUT=64
# CONFIG_RCU_FANOUT_EXACT is not set
CONFIG_TREE_RCU_TRACE=y
CONFIG_PROVE_RCU=y
# CONFIG_PROVE_RCU_REPEATEDLY is not set
# CONFIG_SPARSE_RCU_POINTER is not set
# CONFIG_RCU_TORTURE_TEST is not set
CONFIG_RCU_CPU_STALL_DETECTOR=y
CONFIG_RCU_CPU_STALL_TIMEOUT=60
CONFIG_RCU_CPU_STALL_DETECTOR_RUNNABLE=y
CONFIG_RCU_CPU_STALL_VERBOSE=y


2011/3/31 tip-bot for Davidlohr Bueso <dave@gnu.org>:
> Commit-ID:  706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
> Gitweb:     http://git.kernel.org/tip/706453b0d1fabf4dccbcffa29356d0dd6ab9afb9
> Author:     Davidlohr Bueso <dave@gnu.org>
> AuthorDate: Wed, 30 Mar 2011 23:13:24 -0300
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Thu, 31 Mar 2011 11:06:08 +0200
>
> x86, mce: Fix RCU lockdep from mce_poll()
>
> Based on the following message:
>
>  ===================================================
>  [ INFO: suspicious rcu_dereference_check() usage. ]
>  ---------------------------------------------------
>  arch/x86/kernel/cpu/mcheck/mce.c:1628 invoked
>  rcu_dereference_check() without protection!
>
>  other info that might help us debug this:
>
>  rcu_scheduler_active = 1, debug_locks = 1
>  no locks held by mcelog/2350.
>
>  stack backtrace:
>  Pid: 2350, comm: mcelog Tainted: G        W   2.6.37-rc1+ #7
>  Call Trace:
>   [<ffffffff8108e6d4>] lockdep_rcu_dereference+0xa4/0xc0
>   [<ffffffff810189e9>] mce_poll+0xa9/0xd0
>   [<ffffffff81160585>] do_sys_poll+0x275/0x550
>   [<ffffffff8115f0e0>] ? __pollwait+0x0/0xf0
>   [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
>   [<ffffffff8115f1d0>] ? pollwake+0x0/0x60
>   [<ffffffff8130431c>] ? rcu_read_lock_held+0x2c/0x30
>   [<ffffffff8130549a>] ? radix_tree_lookup_element+0xda/0x100
>   [<ffffffff81121f08>] ? __do_fault+0x128/0x470
>   [<ffffffff81100bdb>] ? filemap_fault+0xdb/0x4e0
>   [<ffffffff810ffe75>] ? unlock_page+0x25/0x30
>   [<ffffffff81069ddf>] ? sigprocmask+0x3f/0x100
>   [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
>   [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
>   [<ffffffff8108ff4d>] ? trace_hardirqs_on+0xd/0x10
>   [<ffffffff8183672b>] ? _raw_spin_unlock_irq+0x2b/0x60
>   [<ffffffff811608a7>] sys_ppoll+0x47/0x190
>   [<ffffffff8108fefd>] ? trace_hardirqs_on_caller+0x13d/0x180
>   [<ffffffff81835d39>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>   [<ffffffff810030eb>] system_call_fastpath+0x16/0x1b
>
> This code is simply checking the value of the pointer, and
> therefore need not protect any actual dereferences. Replace
> rcu_dereference_check_mce() with rcu_dereference_index_check().
>
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Zdenek Kabelac <zdenek.kabelac@gmail.com>
> Cc: Andi Kleen <andi@firstfloor.org>
> LKML-Reference: <1301537604.2140.21.camel@offworld>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 5a05ef6..a2d664f 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1626,7 +1626,7 @@ out:
>  static unsigned int mce_poll(struct file *file, poll_table *wait)
>  {
>        poll_wait(file, &mce_wait, wait);
> -       if (rcu_dereference_check_mce(mcelog.next))
> +       if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
>                return POLLIN | POLLRDNORM;
>        if (!mce_apei_read_done && apei_check_mce())
>                return POLLIN | POLLRDNORM;
>

Re: [tip:x86/urgent] x86, mce: Fix RCU lockdep from mce_poll()

[Paul E. McKenney]

On Thu, Mar 31, 2011 at 12:03:48PM +0200, Zdenek Kabelac wrote:
> However on my machine - I do not a see a difference ?
> 
> I've this patch applied - and it still gives me the same error.
> (using 6aba74f2791287ec407e0f92487a725a25908067 and this patch)
> 
> Here are my RCU config options:
> 
> # grep RCU .config
> # RCU Subsystem
> CONFIG_TREE_PREEMPT_RCU=y
> CONFIG_PREEMPT_RCU=y
> CONFIG_RCU_TRACE=y
> CONFIG_RCU_FANOUT=64
> # CONFIG_RCU_FANOUT_EXACT is not set
> CONFIG_TREE_RCU_TRACE=y
> CONFIG_PROVE_RCU=y
> # CONFIG_PROVE_RCU_REPEATEDLY is not set
> # CONFIG_SPARSE_RCU_POINTER is not set
> # CONFIG_RCU_TORTURE_TEST is not set
> CONFIG_RCU_CPU_STALL_DETECTOR=y
> CONFIG_RCU_CPU_STALL_TIMEOUT=60
> CONFIG_RCU_CPU_STALL_DETECTOR_RUNNABLE=y
> CONFIG_RCU_CPU_STALL_VERBOSE=y

What idiot created that patch, anyway???  ;-)

Here is a patch that is much more likely to do something useful.
Could you please test it?  It is on top of the patch you just tested.

							Thanx, Paul

------------------------------------------------------------------------

rcu: create new rcu_access_index() and use in mce

The MCE subsystem needs to sample an RCU-protected index outside of
any protection for that index.  If this was a pointer, we would use
rcu_access_pointer(), but there is no corresponding rcu_access_index().
This commit therefore creates an rcu_access_index() and applies it
to MCE.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a2d664f..3385ea2 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1626,7 +1626,7 @@ out:
 static unsigned int mce_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &mce_wait, wait);
-	if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
+	if (rcu_access_index(mcelog.next))
 		return POLLIN | POLLRDNORM;
 	if (!mce_apei_read_done && apei_check_mce())
 		return POLLIN | POLLRDNORM;
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index af56148..ff422d2 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -339,6 +339,12 @@ extern int rcu_my_thread_group_empty(void);
 		((typeof(*p) __force __kernel *)(p)); \
 	})
 
+#define __rcu_access_index(p, space) \
+	({ \
+		typeof(p) _________p1 = ACCESS_ONCE(p); \
+		rcu_dereference_sparse(p, space); \
+		(_________p1); \
+	})
 #define __rcu_dereference_index_check(p, c) \
 	({ \
 		typeof(p) _________p1 = ACCESS_ONCE(p); \
@@ -429,6 +435,20 @@ extern int rcu_my_thread_group_empty(void);
 #define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
 
 /**
+ * rcu_access_index() - fetch RCU index with no dereferencing
+ * @p: The index to read
+ *
+ * Return the value of the specified RCU-protected index, but omit the
+ * smp_read_barrier_depends() and keep the ACCESS_ONCE().  This is useful
+ * when the value of this index is accessed, but the index is not
+ * dereferenced, for example, when testing an RCU-protected index against
+ * -1.  Although rcu_access_index() may also be used in cases where
+ * update-side locks prevent the value of the index from changing, you
+ * should instead use rcu_dereference_index_protected() for this use case.
+ */
+#define rcu_access_index(p) __rcu_access_index((p), __rcu)
+
+/**
  * rcu_dereference_index_check() - rcu_dereference for indices with debug checking
  * @p: The pointer to read, prior to dereferencing
  * @c: The conditions under which the dereference will take place

Re: [tip:x86/urgent] x86, mce: Fix RCU lockdep from mce_poll()

[Zdenek Kabelac]

2011/3/31 Paul E. McKenney <paulmck@linux.vnet.ibm.com>:
> On Thu, Mar 31, 2011 at 12:03:48PM +0200, Zdenek Kabelac wrote:
>> However on my machine - I do not a see a difference ?
>>
>> I've this patch applied - and it still gives me the same error.
>> (using 6aba74f2791287ec407e0f92487a725a25908067 and this patch)
>>
>> Here are my RCU config options:
>>
>> # grep RCU .config
>> # RCU Subsystem
>> CONFIG_TREE_PREEMPT_RCU=y
>> CONFIG_PREEMPT_RCU=y
>> CONFIG_RCU_TRACE=y
>> CONFIG_RCU_FANOUT=64
>> # CONFIG_RCU_FANOUT_EXACT is not set
>> CONFIG_TREE_RCU_TRACE=y
>> CONFIG_PROVE_RCU=y
>> # CONFIG_PROVE_RCU_REPEATEDLY is not set
>> # CONFIG_SPARSE_RCU_POINTER is not set
>> # CONFIG_RCU_TORTURE_TEST is not set
>> CONFIG_RCU_CPU_STALL_DETECTOR=y
>> CONFIG_RCU_CPU_STALL_TIMEOUT=60
>> CONFIG_RCU_CPU_STALL_DETECTOR_RUNNABLE=y
>> CONFIG_RCU_CPU_STALL_VERBOSE=y
>
> What idiot created that patch, anyway???  ;-)
>
> Here is a patch that is much more likely to do something useful.
> Could you please test it?  It is on top of the patch you just tested.
>
>                                                        Thanx, Paul
>
> ------------------------------------------------------------------------
>
> rcu: create new rcu_access_index() and use in mce
>
> The MCE subsystem needs to sample an RCU-protected index outside of
> any protection for that index.  If this was a pointer, we would use
> rcu_access_pointer(), but there is no corresponding rcu_access_index().
> This commit therefore creates an rcu_access_index() and applies it
> to MCE.
>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index a2d664f..3385ea2 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -1626,7 +1626,7 @@ out:
>  static unsigned int mce_poll(struct file *file, poll_table *wait)
>  {
>        poll_wait(file, &mce_wait, wait);
> -       if (rcu_dereference_index_check(mcelog.next, rcu_read_lock_sched_held()))
> +       if (rcu_access_index(mcelog.next))
>                return POLLIN | POLLRDNORM;
>        if (!mce_apei_read_done && apei_check_mce())
>                return POLLIN | POLLRDNORM;
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index af56148..ff422d2 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -339,6 +339,12 @@ extern int rcu_my_thread_group_empty(void);
>                ((typeof(*p) __force __kernel *)(p)); \
>        })
>
> +#define __rcu_access_index(p, space) \
> +       ({ \
> +               typeof(p) _________p1 = ACCESS_ONCE(p); \
> +               rcu_dereference_sparse(p, space); \
> +               (_________p1); \
> +       })
>  #define __rcu_dereference_index_check(p, c) \
>        ({ \
>                typeof(p) _________p1 = ACCESS_ONCE(p); \
> @@ -429,6 +435,20 @@ extern int rcu_my_thread_group_empty(void);
>  #define rcu_dereference_raw(p) rcu_dereference_check(p, 1) /*@@@ needed? @@@*/
>
>  /**
> + * rcu_access_index() - fetch RCU index with no dereferencing
> + * @p: The index to read
> + *
> + * Return the value of the specified RCU-protected index, but omit the
> + * smp_read_barrier_depends() and keep the ACCESS_ONCE().  This is useful
> + * when the value of this index is accessed, but the index is not
> + * dereferenced, for example, when testing an RCU-protected index against
> + * -1.  Although rcu_access_index() may also be used in cases where
> + * update-side locks prevent the value of the index from changing, you
> + * should instead use rcu_dereference_index_protected() for this use case.
> + */
> +#define rcu_access_index(p) __rcu_access_index((p), __rcu)
> +
> +/**
>  * rcu_dereference_index_check() - rcu_dereference for indices with debug checking
>  * @p: The pointer to read, prior to dereferencing
>  * @c: The conditions under which the dereference will take place
>

Works for me.

Tested-by: Zdenek Kabelac <zkabelac@redhat.com>

Zdenek

Re: [tip:x86/urgent] x86, mce: Fix RCU lockdep from mce_poll()

[Paul E. McKenney]

On Thu, Mar 31, 2011 at 11:32:20PM +0200, Zdenek Kabelac wrote:
> 2011/3/31 Paul E. McKenney <paulmck@linux.vnet.ibm.com>:
> > On Thu, Mar 31, 2011 at 12:03:48PM +0200, Zdenek Kabelac wrote:

[ . . . ]

> > +/**
> >  * rcu_dereference_index_check() - rcu_dereference for indices with debug checking
> >  * @p: The pointer to read, prior to dereferencing
> >  * @c: The conditions under which the dereference will take place
> >
> 
> Works for me.
> 
> Tested-by: Zdenek Kabelac <zkabelac@redhat.com>

Thank you, Zdenek!

Andi, did you want to carry this patch, or would you prefer that I did so?
If the latter, could you please revert the busted patch?

							Thanx, Paul