Race condition between aio_complete and aio_read_evt

Race condition between aio_complete and aio_read_evt

[Chen, Kenneth W]

[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]

We hit a memory ordering race condition on AIO ring buffer tail pointer
between function aio_complete() and aio_read_evt().

What happens is that on an architecture that has a relaxed memory
ordering model like IPF(ia64), explicit memory barrier is required in a
SMP execution environment.  Considering the following case:

1 CPU is executing a tight loop of aio_read_evt.  It is pulling event
off the ring buffer.  During that loop, another CPU is executing
aio_complete() where it is putting event into the ring buffer and then
update the tail pointer.  However, due to relaxed memory ordering model,
the tail pointer can be visible before the actual event is being
updated.  So the other CPU sees the updated tail pointer but picks up a
staled event data.

A memory barrier is required in this case between the event data and
tail pointer update.  Same is true for the head pointer but the window
of the race condition is nil.  For function correctness, it is fixed
here as well.

By the way, this bug is fixed in the major distributor's kernel on 2.4.x
kernel series for a while, but somehow hasn't been propagated to 2.5
kernel yet.

The patch is relative to 2.5.74.

- Ken

[-- Attachment #2: aio.memorder.patch --]
[-- Type: application/octet-stream, Size: 1003 bytes --]

diff -Nur linux-2.5.74/fs/aio.c linux-2.5.74.aio/fs/aio.c
--- linux-2.5.74/fs/aio.c	Sun Jun 22 11:32:43 2003
+++ linux-2.5.74.aio/fs/aio.c	Tue Jul  8 14:49:27 2003
@@ -679,12 +679,11 @@
 	/* after flagging the request as done, we
 	 * must never even look at it again
 	 */
-	barrier();
+	wmb();	/* make event visible before updating tail */
 
 	info->tail = tail;
 	ring->tail = tail;
 
-	wmb();
 	put_aio_ring_event(event, KM_IRQ0);
 	kunmap_atomic(ring, KM_IRQ1);
 
@@ -721,7 +720,7 @@
 	dprintk("in aio_read_evt h%lu t%lu m%lu\n",
 		 (unsigned long)ring->head, (unsigned long)ring->tail,
 		 (unsigned long)ring->nr);
-	barrier();
+
 	if (ring->head == ring->tail)
 		goto out;
 
@@ -732,7 +731,7 @@
 		struct io_event *evp = aio_ring_event(info, head, KM_USER1);
 		*ent = *evp;
 		head = (head + 1) % info->nr;
-		barrier();
+		mb();	/* finish reading the event before updatng the head */
 		ring->head = head;
 		ret = 1;
 		put_aio_ring_event(evp, KM_USER1);

Re: Race condition between aio_complete and aio_read_evt

[Stephen Hemminger]

On Tue, 8 Jul 2003 14:52:28 -0700
"Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:

> We hit a memory ordering race condition on AIO ring buffer tail pointer
> between function aio_complete() and aio_read_evt().
> 
> What happens is that on an architecture that has a relaxed memory
> ordering model like IPF(ia64), explicit memory barrier is required in a
> SMP execution environment.  Considering the following case:
> 
> 1 CPU is executing a tight loop of aio_read_evt.  It is pulling event
> off the ring buffer.  During that loop, another CPU is executing
> aio_complete() where it is putting event into the ring buffer and then
> update the tail pointer.  However, due to relaxed memory ordering model,
> the tail pointer can be visible before the actual event is being
> updated.  So the other CPU sees the updated tail pointer but picks up a
> staled event data.
> 
> A memory barrier is required in this case between the event data and
> tail pointer update.  Same is true for the head pointer but the window
> of the race condition is nil.  For function correctness, it is fixed
> here as well.
> 
> By the way, this bug is fixed in the major distributor's kernel on 2.4.x
> kernel series for a while, but somehow hasn't been propagated to 2.5
> kernel yet.
> 
> The patch is relative to 2.5.74.
> 
> - Ken
> 

Make those smp_* memory barrier's because they don't matter on UP.

RE: Race condition between aio_complete and aio_read_evt

[Chen, Kenneth W]

> From: Stephen Hemminger [mailto:shemminger@osdl.org] 
> Sent: Tuesday, July 08, 2003 4:00 PM
> Subject: Re: Race condition between aio_complete and aio_read_evt

> Make those smp_* memory barrier's because they don't matter on UP.

Certainly, thanks for pointing that out.  I was about to say that as
well.

- Ken