irq_work: Fix ordering issue

We need to convert flags to atomic_t in order to later fix an ordering issue on cmpxchg() failure. This will allow us to use atomic_fetch_or(). Also that clarify the nature of those flags.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

---
 include/linux/irq_work.h | 10 +++++++---
 kernel/irq_work.c        | 18 +++++++++---------
 kernel/printk/printk.c   |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

Quoted 9 lines · Frederic Weisbecker

We need to convert flags to atomic_t in order to later fix an ordering issue on cmpxchg() failure. This will allow us to use atomic_fetch_or(). Also that clarify the nature of those flags.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

Quoted 5 lines

---
 include/linux/irq_work.h | 10 +++++++---
 kernel/irq_work.c        | 18 +++++++++---------
 kernel/printk/printk.c   |  2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

You missed the irq_work use in kernel/bpf/stackmap.c - see the fix below.

Thanks,

Ingo

When irq_work_claim() finds IRQ_WORK_PENDING flag already set, we just return and don't raise a new IPI. We expect the destination to see and handle our latest updades thanks to the pairing atomic_xchg() in irq_work_run_list().

But cmpxchg() doesn't guarantee a full memory barrier upon failure. So it's possible that the destination misses our latest updates.

So use atomic_fetch_or() instead that is unconditionally fully ordered and also performs exactly what we want here and simplify the code.

Quoted 5 lines · Frederic Weisbecker

When irq_work_claim() finds IRQ_WORK_PENDING flag already set, we just return and don't raise a new IPI. We expect the destination to see and handle our latest updades thanks to the pairing atomic_xchg() in irq_work_run_list().

Quoted 5 lines

But cmpxchg() doesn't guarantee a full memory barrier upon failure. So it's possible that the destination misses our latest updates.

So use atomic_fetch_or() instead that is unconditionally fully ordered and also performs exactly what we want here and simplify the code.

Just curious, how was this bug found - in the wild, or via code review?

Instead of fetching the value of flags and perform an xchg() to clear a bit, just use atomic_fetch_andnot() that is more suitable to do that job in one operation while keeping the full ordering.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

---
  kernel/irq_work.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

kernel/irq_work.c +2 -2

index 255454a48346..49c53f80a13a 100644
@@ -34,7 +34,7 @@ static bool irq_work_claim(struct irq_work *work)
  	oflags = atomic_fetch_or(IRQ_WORK_CLAIMED, &work->flags);
  	/*
  	 * If the work is already pending, no need to raise the IPI.
-	 * The pairing atomic_xchg() in irq_work_run() makes sure
+	 * The pairing atomic_fetch_andnot() in irq_work_run() makes sure
  	 * everything we did before is visible.
  	 */
  	if (oflags & IRQ_WORK_PENDING)

@@ -135,7 +135,6 @@ static void irq_work_run_list(struct llist_head *list)
  {
  	struct irq_work *work, *tmp;
  	struct llist_node *llnode;
-	int flags;
 
  	BUG_ON(!irqs_disabled());
 

@@ -144,6 +143,7 @@ static void irq_work_run_list(struct llist_head *list)
 
  	llnode = llist_del_all(list);
  	llist_for_each_entry_safe(work, tmp, llnode, llnode) {
+		int flags;
  		/*
  		 * Clear the PENDING bit, after this point the @work
  		 * can be re-used.

Quoted 43 lines · Frederic Weisbecker

Instead of fetching the value of flags and perform an xchg() to clear a bit, just use atomic_fetch_andnot() that is more suitable to do that job in one operation while keeping the full ordering.

Signed-off-by Frederic Weisbecker <[email protected]>

Cc Paul E. McKenney <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Ingo Molnar <[email protected]>

---
  kernel/irq_work.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

kernel/irq_work.c +2 -2

index 255454a48346..49c53f80a13a 100644
@@ -34,7 +34,7 @@ static bool irq_work_claim(struct irq_work *work)
  	oflags = atomic_fetch_or(IRQ_WORK_CLAIMED, &work->flags);
  	/*
  	 * If the work is already pending, no need to raise the IPI.
-	 * The pairing atomic_xchg() in irq_work_run() makes sure
+	 * The pairing atomic_fetch_andnot() in irq_work_run() makes sure
  	 * everything we did before is visible.
  	 */
  	if (oflags & IRQ_WORK_PENDING)

@@ -135,7 +135,6 @@ static void irq_work_run_list(struct llist_head *list)
  {
  	struct irq_work *work, *tmp;
  	struct llist_node *llnode;
-	int flags;
 
  	BUG_ON(!irqs_disabled());
 

@@ -144,6 +143,7 @@ static void irq_work_run_list(struct llist_head *list)
 
  	llnode = llist_del_all(list);
  	llist_for_each_entry_safe(work, tmp, llnode, llnode) {
+		int flags;
  		/*
  		 * Clear the PENDING bit, after this point the @work
  		 * can be re-used.

kernel/irq_work.c static void irq_work_run_list(struct llist_head *list) · line 157

Quoted 9 lines

kernel/irq_work.c +1 -2

@@ -151,8 +151,7 @@ static void irq_work_run_list(struct llist_head *list)
  		 * to claim that work don't rely on us to handle their data
  		 * while we are in the middle of the func.
  		 */
-		flags = atomic_read(&work->flags) & ~IRQ_WORK_PENDING;
-		atomic_xchg(&work->flags, flags);
+		flags = atomic_fetch_andnot(IRQ_WORK_PENDING, &work->flags);
 
  		work->func(work);
  		/*

This breaks switching between cpufreq governors in linux-next on arm64 and various other stuff. The fix in this email doesn't compile:

     https://lkml.org/lkml/2019/11/12/622

I assume you meant "&= ~" instead of "~=", this seems to work:

kernel/irq_work.c +1 -0

index 49c53f80a13a..828cc30774bc 100644
@@ -156,10 +156,11 @@ static void irq_work_run_list(struct llist_head *list)
                 work->func(work);
                 /*
                  * Clear the BUSY bit and return to the free state if
                  * no-one else claimed it meanwhile.
                  */
+               flags &= ~IRQ_WORK_PENDING;
                 (void)atomic_cmpxchg(&work->flags, flags, flags & 
 ~IRQ_WORK_BUSY);
         }
  }

kernel/irq_work.c +2 -0

index 49c53f80a13a..b709ab05cbfd 100644
@@ -34,8 +34,8 @@ static bool irq_work_claim(struct irq_work *work)
 	oflags = atomic_fetch_or(IRQ_WORK_CLAIMED, &work->flags);
 	/*
 	 * If the work is already pending, no need to raise the IPI.
+	 * The pairing atomic_andnot() followed by a barrier in irq_work_run()
+	 * makes sure everything we did before is visible.
 	 */
 	if (oflags & IRQ_WORK_PENDING)
 		return false;

kernel/irq_work.c +2 -1

 		work->func(work);
 		/*
 		 * Clear the BUSY bit and return to the free state if
 		 * no-one else claimed it meanwhile.
 		 */
-		(void)atomic_cmpxchg(&work->flags, flags, flags & ~IRQ_WORK_BUSY);
+		(void)atomic_cmpxchg(&work->flags, flags & ~IRQ_WORK_PENDING,
+				     flags & ~IRQ_WORK_CLAIMED);
 	}
 }
 

Signature · 1 line

2.23.0

Quoted 15 lines · Frederic Weisbecker

kernel/irq_work.c +2 -0

index 49c53f80a13a..b709ab05cbfd 100644
@@ -34,8 +34,8 @@ static bool irq_work_claim(struct irq_work *work)
 	oflags = atomic_fetch_or(IRQ_WORK_CLAIMED, &work->flags);
 	/*
 	 * If the work is already pending, no need to raise the IPI.
+	 * The pairing atomic_andnot() followed by a barrier in irq_work_run()
+	 * makes sure everything we did before is visible.
 	 */
 	if (oflags & IRQ_WORK_PENDING)
 		return false;

kernel/irq_work.c static void irq_work_run_list(struct llist_head *list) · line 155

Quoted 6 lines

kernel/irq_work.c +2 -1

@@ -151,14 +151,16 @@ static void irq_work_run_list(struct llist_head *list)
 		 * to claim that work don't rely on us to handle their data
 		 * while we are in the middle of the func.
 		 */
-		flags = atomic_fetch_andnot(IRQ_WORK_PENDING, &work->flags);
+		atomic_andnot(IRQ_WORK_PENDING, &work->flags);
+		smp_mb__after_atomic();

I think I'm prefering you use:

		flags = atomic_fetch_andnot_acquire(IRQ_WORK_PENDING, &work->flags);

Also, I'm cursing at myself for the horrible comments here.

Quoted 15 lines

kernel/irq_work.c +2 -1

 		work->func(work);
 		/*
 		 * Clear the BUSY bit and return to the free state if
 		 * no-one else claimed it meanwhile.
 		 */
-		(void)atomic_cmpxchg(&work->flags, flags, flags & ~IRQ_WORK_BUSY);
+		(void)atomic_cmpxchg(&work->flags, flags & ~IRQ_WORK_PENDING,
+				     flags & ~IRQ_WORK_CLAIMED);
 	}
 }
 

Signature · 1 line

2.23.0