virtio: fix virtio_net xmit of freed skb bug
authorMark McLoughlin <markmc@redhat.com>
Tue, 27 May 2008 11:06:26 +0000 (12:06 +0100)
committerRusty Russell <rusty@rustcorp.com.au>
Fri, 25 Jul 2008 02:06:00 +0000 (12:06 +1000)
On Mon, 2008-05-26 at 17:42 +1000, Rusty Russell wrote:
> If we fail to transmit a packet, we assume the queue is full and put
> the skb into last_xmit_skb.  However, if more space frees up before we
> xmit it, we loop, and the result can be transmitting the same skb twice.
>
> Fix is simple: set skb to NULL if we've used it in some way, and check
> before sending.
...
> diff -r 564237b31993 drivers/net/virtio_net.c
> --- a/drivers/net/virtio_net.c Mon May 19 12:22:00 2008 +1000
> +++ b/drivers/net/virtio_net.c Mon May 19 12:24:58 2008 +1000
> @@ -287,21 +287,25 @@ again:
>   free_old_xmit_skbs(vi);
>
>   /* If we has a buffer left over from last time, send it now. */
> - if (vi->last_xmit_skb) {
> + if (unlikely(vi->last_xmit_skb)) {
>   if (xmit_skb(vi, vi->last_xmit_skb) != 0) {
>   /* Drop this skb: we only queue one. */
>   vi->dev->stats.tx_dropped++;
>   kfree_skb(skb);
> + skb = NULL;
>   goto stop_queue;
>   }
>   vi->last_xmit_skb = NULL;

With this, may drop an skb and then later in the function discover that
we could have sent it after all. Poor wee skb :)

How about the incremental patch below?

Cheers,
Mark.

Subject: [PATCH] virtio_net: Delay dropping tx skbs

Currently we drop the skb in start_xmit() if we have a
queued buffer and fail to transmit it.

However, if we delay dropping it until we've stopped the
queue and enabled the tx notification callback, then there
is a chance space might become available for it.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
drivers/net/virtio_net.c

index c28d7cb2035b1dbcb5425aaeff55294d3f4eb786..06d5c43bb2076950947992f55d0da7200cb70fb1 100644 (file)
@@ -335,16 +335,11 @@ again:
        free_old_xmit_skbs(vi);
 
        /* If we has a buffer left over from last time, send it now. */
-       if (unlikely(vi->last_xmit_skb)) {
-               if (xmit_skb(vi, vi->last_xmit_skb) != 0) {
-                       /* Drop this skb: we only queue one. */
-                       vi->dev->stats.tx_dropped++;
-                       kfree_skb(skb);
-                       skb = NULL;
-                       goto stop_queue;
-               }
-               vi->last_xmit_skb = NULL;
-       }
+       if (unlikely(vi->last_xmit_skb) &&
+           xmit_skb(vi, vi->last_xmit_skb) != 0)
+               goto stop_queue;
+
+       vi->last_xmit_skb = NULL;
 
        /* Put new one in send queue and do transmit */
        if (likely(skb)) {
@@ -370,6 +365,11 @@ stop_queue:
                netif_start_queue(dev);
                goto again;
        }
+       if (skb) {
+               /* Drop this skb: we only queue one. */
+               vi->dev->stats.tx_dropped++;
+               kfree_skb(skb);
+       }
        goto done;
 }