percpu_rwsem: fix missed wakeup due to reordering of load

In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already acquired for read operation by P1 using percpu_down_read().

Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2 is in the process of acquiring cpu_hotplug_lock.

P1 P2
percpu_up_read() path percpu_down_write() path

rcu_sync_enter()
//gp_state=GP_PASSED

rcu_sync_is_idle() //returns false down_write(rw_sem)

__percpu_up_read()

[L] task = rcu_dereference(w->task) //NULL

smp_rmb() [S] w->task = current

                                            smp_mb()

                                           [L] readers_active_check() 

//fails
schedule()

[S] __this_cpu_dec(read_count)

Since load of task can result in NULL. This can lead to missed wakeup in rcuwait_wake_up(). Above sequence violated the following constraint in rcuwait_wake_up():

WAIT WAKE
[S] tsk = current [S] cond = true
MB (A) MB (B)
[L] cond [L] tsk

Quoted 26 lines · Prateek Sood

In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already acquired for read operation by P1 using percpu_down_read().

Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2 is in the process of acquiring cpu_hotplug_lock.

P1                                               P2 percpu_up_read() path                      percpu_down_write() path

                                          rcu_sync_enter() //gp_state=GP_PASSED

rcu_sync_is_idle() //returns false        down_write(rw_sem)

__percpu_up_read()

[L] task = rcu_dereference(w->task) //NULL

smp_rmb()                                  [S] w->task = current

                                            smp_mb()

                                           [L] readers_active_check() //fails schedule()

[S] __this_cpu_dec(read_count)

Quoted 9 lines

Since load of task can result in NULL. This can lead to missed wakeup in rcuwait_wake_up(). Above sequence violated the following constraint in rcuwait_wake_up():

     WAIT                WAKE
[S] tsk = current      [S] cond = true
MB (A)                        MB (B)
[L] cond          [L] tsk

Hmm yeah we don't want rcu_wake_up() to get hoisted over the __this_cpu_dec(read_count). The smp_rmb() does not make sense to me here in the first place. Did you run into this scenario by code inspection or you actually it the issue?

Thanks,
Davidlohr

Quoted 37 lines · Davidlohr Bueso

Quoted 26 lines · Prateek Sood

In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already acquired for read operation by P1 using percpu_down_read().

Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2 is in the process of acquiring cpu_hotplug_lock.

P1                                               P2 percpu_up_read() path                      percpu_down_write() path

                                          rcu_sync_enter() //gp_state=GP_PASSED

rcu_sync_is_idle() //returns false        down_write(rw_sem)

__percpu_up_read()

[L] task = rcu_dereference(w->task) //NULL

smp_rmb()                                  [S] w->task = current

                                            smp_mb()

                                           [L] readers_active_check() //fails schedule()

[S] __this_cpu_dec(read_count)

Quoted 9 lines

Since load of task can result in NULL. This can lead to missed wakeup in rcuwait_wake_up(). Above sequence violated the following constraint in rcuwait_wake_up():

     WAIT                WAKE
[S] tsk = current      [S] cond = true
MB (A)                        MB (B)
[L] cond          [L] tsk

Quoted 4 lines

Hmm yeah we don't want rcu_wake_up() to get hoisted over the __this_cpu_dec(read_count). The smp_rmb() does not make sense to me here in the first place. Did you run into this scenario by code inspection or you actually it the issue?

Thanks,
Davidlohr

I have checked one issue where it seems that cpu hotplug code path is not able to get cpu_hotplug_lock in write mode and there is a reader pending for cpu hotplug path to release percpu_rw_semaphore->rwsem to acquire cpu_hotplug_lock. This caused a deadlock.

From code inspection also it seems to be not adhering to arm64 smp_rmb() constraint of load/load-store ordering guarantee.

Thanks,
Prateek

Please confirm if the suspicion of smp_rmb is correct. IMO, it should be smp_mb() translating to dmb ish.

P1 is releaseing the cpu_hotplug_lock and P2 is acquiring cpu_hotplug_lock.

P1 P2
percpu_up_read() path percpu_down_write() path

rcu_sync_enter() //gp_state=GP_PASSED

rcu_sync_is_idle() //returns false down_write(rw_sem)

__percpu_up_read()

[L] task = rcu_dereference(w->task) //NULL

smp_rmb() [S] w->task = current

smp_mb()

[L] readers_active_check() //fails schedule()

[S] __this_cpu_dec(read_count)

Since load of task can result in NULL, it can lead to missed wakeup in rcuwait_wake_up(). Above sequence violated the following constraint in rcuwait_wake_up():

WAIT WAKE
[S] tsk = current [S] cond = true
MB (A) MB (B)
[L] cond [L] tsk

This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering of load before barrier with load and store after barrier for arm64 architecture. Here the requirement is to order store before smp_rmb() with load after the smp_rmb().

For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier (smp_mb) is required to complete the constraint of rcuwait_wake_up().

Signed-off-by Prateek Sood <[email protected]>

Acked-by Davidlohr Bueso <[email protected]>

---
 kernel/exit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/exit.c +2 -2

index ac1a814..696e0e1 100644
@@ -298,7 +298,7 @@ void rcuwait_wake_up(struct rcuwait *w)
 	/*
 	 * Order condition vs @task, such that everything prior to the load
 	 * of @task is visible. This is the condition as to why the user called
-	 * rcuwait_trywake() in the first place. Pairs with set_current_state()
+	 * rcuwait_wake_up() in the first place. Pairs with set_current_state()
 	 * barrier (A) in rcuwait_wait_event().
 	 *
 	 *    WAIT                WAKE

@@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
 	 *        MB (A)	      MB (B)
 	 *    [L] cond		  [L] tsk
 	 */
-	smp_rmb(); /* (B) */
+	smp_mb(); /* (B) */
 
 	/*
 	 * Avoid using task_rcu_dereference() magic as long as we are careful,

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already acquired for read operation by P1 using percpu_down_read().

Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2 is in the process of acquiring cpu_hotplug_lock.

P1 P2
percpu_up_read() path percpu_down_write() path

rcu_sync_enter() //gp_state=GP_PASSED

rcu_sync_is_idle() //returns false down_write(rw_sem)

__percpu_up_read()

[L] task = rcu_dereference(w->task) //NULL

smp_rmb() [S] w->task = current

smp_mb()

[L] readers_active_check() //fails schedule()

[S] __this_cpu_dec(read_count)

Since load of task can result in NULL. This can lead to missed wakeup in rcuwait_wake_up(). Above sequence violated the following constraint in rcuwait_wake_up():

WAIT WAKE
[S] tsk = current [S] cond = true
MB (A) MB (B)
[L] cond [L] tsk

This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering of load before barrier with load and store after barrier for arm64 architecture. Here the requirement is to order store before smp_rmb() with load after the smp_rmb().

For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier (smp_mb) is required to complete the constraint of rcuwait_wake_up().

Signed-off-by Prateek Sood <[email protected]>

---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/exit.c +1 -1

index f1d74f0..a10820d 100644
@@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
 	 *        MB (A)	      MB (B)
 	 *    [L] cond		  [L] tsk
 	 */
-	smp_rmb(); /* (B) */
+	smp_mb(); /* (B) */
 
 	/*
 	 * Avoid using task_rcu_dereference() magic as long as we are careful,

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Quoted 40 lines · Prateek Sood

In a scenario where cpu_hotplug_lock percpu_rw_semaphore is already acquired for read operation by P1 using percpu_down_read().

Now we have P1 in the path of releaseing the cpu_hotplug_lock and P2 is in the process of acquiring cpu_hotplug_lock.

P1 P2
percpu_up_read() path percpu_down_write() path

rcu_sync_enter() //gp_state=GP_PASSED

rcu_sync_is_idle() //returns false down_write(rw_sem)

__percpu_up_read()

[L] task = rcu_dereference(w->task) //NULL

smp_rmb() [S] w->task = current

smp_mb()

[L] readers_active_check() //fails schedule()

[S] __this_cpu_dec(read_count)

Since load of task can result in NULL. This can lead to missed wakeup in rcuwait_wake_up(). Above sequence violated the following constraint in rcuwait_wake_up():

WAIT WAKE
[S] tsk = current [S] cond = true
MB (A) MB (B)
[L] cond [L] tsk

This can happen as smp_rmb() in rcuwait_wake_up() will provide ordering of load before barrier with load and store after barrier for arm64 architecture. Here the requirement is to order store before smp_rmb() with load after the smp_rmb().

Quoted 4 lines

For the usage of rcuwait_wake_up() in __percpu_up_read() full barrier (smp_mb) is required to complete the constraint of rcuwait_wake_up().

Signed-off-by Prateek Sood <[email protected]>

I know this is going to sound ridiculous (coming from me or from the Italian that I am), but it looks like we could both work on our English. ;-)

But the fix seems correct to me:

Reviewed-by Andrea Parri <[email protected]>

It might be a good idea to integrate this fix with fixes to the inline comments/annotations: for example, I see that the comment in rcuwait_wake_up() mentions a non-existing rcuwait_trywake();

moreover, the memory-barrier annotation "B" is used also for the smp_mb() preceding the __this_cpu_dec() in __percpu_up_read().

Andrea

Quoted 21 lines

---
 kernel/exit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/exit.c +1 -1

index f1d74f0..a10820d 100644
@@ -306,7 +306,7 @@ void rcuwait_wake_up(struct rcuwait *w)
 	 *        MB (A)	      MB (B)
 	 *    [L] cond		  [L] tsk
 	 */
-	smp_rmb(); /* (B) */
+	smp_mb(); /* (B) */
 
 	/*
 	 * Avoid using task_rcu_dereference() magic as long as we are careful,

Signature · 2 lines

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc., 
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.