From mboxrd@z Thu Jan  1 00:00:00 1970
From: Rusty Russell <rusty@rustcorp.com.au>
Subject: Re: virtio_net: another race with virtio_net and enable_cb
Date: Tue, 27 May 2008 12:36:00 +1000
Message-ID: <200805271236.00439.rusty@rustcorp.com.au>
References: <200805261129.27718.borntraeger@de.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Cc: virtualization@lists.linux-foundation.org, kvm@vger.kernel.org
To: Christian Borntraeger <borntraeger@de.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from ozlabs.org ([203.10.76.45]:48065 "EHLO ozlabs.org"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753553AbYE0CgV (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 26 May 2008 22:36:21 -0400
In-Reply-To: <200805261129.27718.borntraeger@de.ibm.com>
Content-Disposition: inline
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Monday 26 May 2008 19:29:27 Christian Borntraeger wrote:
> Hello Rusty,

Hi Christian,

    As always, you post an interesting bug!

>         /* Out of packets? */
>         if (received < budget) {
>                 netif_rx_complete(vi->dev, napi);
>                 if (unlikely(!vi->rvq->vq_ops->enable_cb(vi->rvq))
>                     && napi_schedule_prep(napi)) {
>                         vi->rvq->vq_ops->disable_cb(vi->rvq);
>                         __netif_rx_schedule(vi->dev, napi);
>                         goto again;
>                 }
>         }

Yes, clearly if napi_schedule_prep() fails (ie. another poll is scheduled), we 
don't disable callbacks, and another poll will be run with callbacks enabled, 
triggering this bug.  It can only happen if the enable_cb succeeds (no new 
work) then a new packet arrives before napi_schedule_prep().

You should be able to verify that you are hitting this window by putting a 
printk in the !enable_cb && !napi_schedule_prep case.

Now, we can't do an unconditional disable_cb(), as it won't prevent this race, 
and would make it worse if it ran after the other poll's enable_cb() and left 
us without a callback to wake us up.  We could do a disable_cb() at the top 
of virtnet_poll, but I certainly don't want to write the comment explaining 
why.

So your fix is correct and sensible.  Applied.

Thanks!
Rusty.