[patch] eventfd/kaio integration fix

[patch] eventfd/kaio integration fix

[Davide Libenzi]

Jeff Roberson discovered a race when using kaio eventfd based 
notifications. This patch fixes the race by moving the notification inside 
the spinlocked section of kaio. The operation is safe since eventfd 
spinlock and kaio one are unrelated.



Signed-off-by: Davide Libenzi <davidel@xmailserver.org>


- Davide


---
 fs/aio.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Index: linux-2.6.mod/fs/aio.c
===================================================================
--- linux-2.6.mod.orig/fs/aio.c	2008-04-08 16:25:27.000000000 -0700
+++ linux-2.6.mod/fs/aio.c	2008-04-09 11:37:10.000000000 -0700
@@ -936,14 +936,6 @@
 		return 1;
 	}
 
-	/*
-	 * Check if the user asked us to deliver the result through an
-	 * eventfd. The eventfd_signal() function is safe to be called
-	 * from IRQ context.
-	 */
-	if (!IS_ERR(iocb->ki_eventfd))
-		eventfd_signal(iocb->ki_eventfd, 1);
-
 	info = &ctx->ring_info;
 
 	/* add a completion event to the ring buffer.
@@ -992,6 +984,15 @@
 	kunmap_atomic(ring, KM_IRQ1);
 
 	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
+
+	/*
+	 * Check if the user asked us to deliver the result through an
+	 * eventfd. The eventfd_signal() function is safe to be called
+	 * from IRQ context.
+	 */
+	if (!IS_ERR(iocb->ki_eventfd))
+		eventfd_signal(iocb->ki_eventfd, 1);
+
 put_rq:
 	/* everything turned out well, dispose of the aiocb. */
 	ret = __aio_put_req(ctx, iocb);

Re: [patch] eventfd/kaio integration fix

[Andrew Morton]

On Wed, 9 Apr 2008 11:45:47 -0700 (PDT)
Davide Libenzi <davidel@xmailserver.org> wrote:

> Jeff Roberson discovered a race when using kaio eventfd based 
> notifications. This patch fixes the race by moving the notification inside 
> the spinlocked section of kaio.

Missing information.

What are the consequences of this race, when it occurs?

> The operation is safe since eventfd 
> spinlock and kaio one are unrelated.

Yes, it's safe from that perspective.

However with this patch applied, we will no longer run eventfd_signal() if
kiocbIsCancelled(iocb).  Convincing is needed, please?

Re: [patch] eventfd/kaio integration fix

[Davide Libenzi]

On Wed, 9 Apr 2008, Andrew Morton wrote:

> On Wed, 9 Apr 2008 11:45:47 -0700 (PDT)
> Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > Jeff Roberson discovered a race when using kaio eventfd based 
> > notifications. This patch fixes the race by moving the notification inside 
> > the spinlocked section of kaio.
> 
> Missing information.
> 
> What are the consequences of this race, when it occurs?

This was described in the original email. I posted a patch back then 
(waiting for Jeff test feedback - that never came), but then I forgot 
about it till now:

http://groups.google.com/group/linux.kernel/browse_thread/thread/e814b54c14198616



> > The operation is safe since eventfd 
> > spinlock and kaio one are unrelated.
> 
> Yes, it's safe from that perspective.
> 
> However with this patch applied, we will no longer run eventfd_signal() if
> kiocbIsCancelled(iocb).  Convincing is needed, please?

This was the intended behaviour. No event was actually *ready*, so no need 
to signal completion of an event.



- Davide

Re: [patch] eventfd/kaio integration fix

[Jeff Roberson]

On Wed, 9 Apr 2008, Davide Libenzi wrote:

> On Wed, 9 Apr 2008, Andrew Morton wrote:
>
>> On Wed, 9 Apr 2008 11:45:47 -0700 (PDT)
>> Davide Libenzi <davidel@xmailserver.org> wrote:
>>
>>> Jeff Roberson discovered a race when using kaio eventfd based
>>> notifications. This patch fixes the race by moving the notification inside
>>> the spinlocked section of kaio.
>>
>> Missing information.
>>
>> What are the consequences of this race, when it occurs?
>
> This was described in the original email. I posted a patch back then
> (waiting for Jeff test feedback - that never came), but then I forgot
> about it till now:
>
> http://groups.google.com/group/linux.kernel/browse_thread/thread/e814b54c14198616
>

I was thinking about stirring this up again myself.  Testing was 
complicated by several factors.  None of them related to this patch. 
However, I feel confident that this has solved our issue.

Jeff

>
>
>>> The operation is safe since eventfd
>>> spinlock and kaio one are unrelated.
>>
>> Yes, it's safe from that perspective.
>>
>> However with this patch applied, we will no longer run eventfd_signal() if
>> kiocbIsCancelled(iocb).  Convincing is needed, please?
>
> This was the intended behaviour. No event was actually *ready*, so no need
> to signal completion of an event.
>
>
>
> - Davide
>
>

Re: [patch] eventfd/kaio integration fix

[Oliver Pinter]

for 2.6.22.y ?

On 4/9/08, Davide Libenzi <davidel@xmailserver.org> wrote:
> Jeff Roberson discovered a race when using kaio eventfd based
> notifications. This patch fixes the race by moving the notification inside
> the spinlocked section of kaio. The operation is safe since eventfd
> spinlock and kaio one are unrelated.
>
>
>
> Signed-off-by: Davide Libenzi <davidel@xmailserver.org>
>
>
> - Davide
>
>
> ---
>  fs/aio.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> Index: linux-2.6.mod/fs/aio.c
> ===================================================================
> --- linux-2.6.mod.orig/fs/aio.c	2008-04-08 16:25:27.000000000 -0700
> +++ linux-2.6.mod/fs/aio.c	2008-04-09 11:37:10.000000000 -0700
> @@ -936,14 +936,6 @@
>  		return 1;
>  	}
>
> -	/*
> -	 * Check if the user asked us to deliver the result through an
> -	 * eventfd. The eventfd_signal() function is safe to be called
> -	 * from IRQ context.
> -	 */
> -	if (!IS_ERR(iocb->ki_eventfd))
> -		eventfd_signal(iocb->ki_eventfd, 1);
> -
>  	info = &ctx->ring_info;
>
>  	/* add a completion event to the ring buffer.
> @@ -992,6 +984,15 @@
>  	kunmap_atomic(ring, KM_IRQ1);
>
>  	pr_debug("added to ring %p at [%lu]\n", iocb, tail);
> +
> +	/*
> +	 * Check if the user asked us to deliver the result through an
> +	 * eventfd. The eventfd_signal() function is safe to be called
> +	 * from IRQ context.
> +	 */
> +	if (!IS_ERR(iocb->ki_eventfd))
> +		eventfd_signal(iocb->ki_eventfd, 1);
> +
>  put_rq:
>  	/* everything turned out well, dispose of the aiocb. */
>  	ret = __aio_put_req(ctx, iocb);
> --
> 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/
>


-- 
Thanks,
Oliver

Re: [patch] eventfd/kaio integration fix

[Davide Libenzi]

On Sun, 13 Apr 2008, Oliver Pinter wrote:

> for 2.6.22.y ?

You may want to talk to either Greg KH or Chris Wright for that.



- Davide

Re: [patch] eventfd/kaio integration fix

[Oliver Pinter]

this race presented in forigen kernels or only in 2.6.25-rcX?

I added this patch for "queue-2.6.22.y"
http://repo.or.cz/w/linux-2.6.22.y-op-patches.git

On 4/14/08, Davide Libenzi <davidel@xmailserver.org> wrote:
> On Sun, 13 Apr 2008, Oliver Pinter wrote:
>
> > for 2.6.22.y ?
>
> You may want to talk to either Greg KH or Chris Wright for that.
>
>
>
> - Davide
>
>
>


-- 
Thanks,
Oliver

Re: [patch] eventfd/kaio integration fix

[Davide Libenzi]

On Mon, 14 Apr 2008, Oliver Pinter wrote:

> this race presented in forigen kernels or only in 2.6.25-rcX?

"Forigen"?
The race is there since day one of kaio+eventfd integration. So basically, 
if your kernel has eventfd, it has also the race.
Needless to say, simple kaio usage w/out eventfd, is fine.



- Davide