Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.

[Jesper Juhl <jesper.juhl@gmail.com>]

On Monday 13 August 2007 21:54:37 Jeremy Fitzhardinge wrote:
> xennet_tx_bug_gc can free the skb before we use it, so make sure we don't.
> 
> Jeff, this is -rc material.
> 
> Signed-off-by: Keir Fraser <keir@xensource.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>
> Cc: Jeff Garzik <jeff@garzik.org>
> 
> diff -r 8bfc43f6d1b0 drivers/net/xen-netfront.c
> --- a/drivers/net/xen-netfront.c	Tue Aug 07 14:26:30 2007 -0700
> +++ b/drivers/net/xen-netfront.c	Mon Aug 13 09:39:15 2007 -0700
> @@ -566,15 +566,16 @@ static int xennet_start_xmit(struct sk_b
>  	if (notify)
>  		notify_remote_via_irq(np->netdev->irq);
>  
> +	np->stats.tx_bytes += skb->len;
> +	np->stats.tx_packets++;
> +
> +	/* Note: It is not safe to access skb after xennet_tx_buf_gc()! */
>  	xennet_tx_buf_gc(dev);
>  
>  	if (!netfront_tx_slot_available(np))
>  		netif_stop_queue(dev);
>  
>  	spin_unlock_irq(&np->tx_lock);
> -
> -	np->stats.tx_bytes += skb->len;
> -	np->stats.tx_packets++;
>  
>  	return 0;
>  
This moves the updating of both tx_bytes and tx_packets inside the 
spinlock, but as far as I can see we only _really_ need to move the 
tx_bytes update. Considering that we generally want to do as little 
work as possible while holding a lock, wouldn't the following be 
slightly better?


Signed-off-by: Keir Fraser <keir@xensource.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>
Cc: Jeff Garzik <jeff@garzik.org>
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 drivers/net/xen-netfront.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 489f69c..640e270 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -566,6 +566,9 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (notify)
 		notify_remote_via_irq(np->netdev->irq);
 
+	np->stats.tx_bytes += skb->len;
+
+	/* Note: It is not safe to access skb after xennet_tx_buf_gc()! */
 	xennet_tx_buf_gc(dev);
 
 	if (!netfront_tx_slot_available(np))
@@ -573,7 +576,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	spin_unlock_irq(&np->tx_lock);
 
-	np->stats.tx_bytes += skb->len;
 	np->stats.tx_packets++;
 
 	return 0;