Xen devel - Jun 2006 - [PATCH][NET] front: cleanup some error paths

There''s a small leak on a couple error paths in setup_device().
While there rearrange the ring setup order slightly to simplify error
path since netif_free() will cleanup once ring_ref is valid.  And use
get_zeroed_page() instead of __get_free_page()/memset().  Handle error if
bind_evtchn_to_irqhandler() fails, as bad info->irq value is likely to
cause oops later.  In create_device(), gnttab_free_grant_references()
is accidentally called twice on tx_head during cleanup from failed
gnttab_alloc_grant_references() on rx_head, which could corrupt
gnttab_free_count.

Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c |   41 +++++++++----------
 1 file changed, 21 insertions(+), 20 deletions(-)

--- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Thu Jun 08 16:51:39
2006 +0100
+++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c	Thu Jun 08 14:49:04
2006 -0400
@@ -338,35 +338,36 @@ static int setup_device(struct xenbus_de
 	info->tx.sring = NULL;
 	info->irq = 0;
 
-	txs = (struct netif_tx_sring *)__get_free_page(GFP_KERNEL);
+	txs = (struct netif_tx_sring *)get_zeroed_page(GFP_KERNEL);
 	if (!txs) {
 		err = -ENOMEM;
 		xenbus_dev_fatal(dev, err, "allocating tx ring page");
 		goto fail;
 	}
-	rxs = (struct netif_rx_sring *)__get_free_page(GFP_KERNEL);
+	SHARED_RING_INIT(txs);
+	FRONT_RING_INIT(&info->tx, txs, PAGE_SIZE);
+
+	err = xenbus_grant_ring(dev, virt_to_mfn(txs));
+	if (err < 0) {
+		free_page((unsigned long)txs);
+		goto fail;
+	}
+	info->tx_ring_ref = err;
+
+	rxs = (struct netif_rx_sring *)get_zeroed_page(GFP_KERNEL);
 	if (!rxs) {
 		err = -ENOMEM;
 		xenbus_dev_fatal(dev, err, "allocating rx ring page");
 		goto fail;
 	}
-	memset(txs, 0, PAGE_SIZE);
-	memset(rxs, 0, PAGE_SIZE);
-
-	SHARED_RING_INIT(txs);
-	FRONT_RING_INIT(&info->tx, txs, PAGE_SIZE);
-
 	SHARED_RING_INIT(rxs);
 	FRONT_RING_INIT(&info->rx, rxs, PAGE_SIZE);
 
-	err = xenbus_grant_ring(dev, virt_to_mfn(txs));
-	if (err < 0)
+	err = xenbus_grant_ring(dev, virt_to_mfn(rxs));
+	if (err < 0) {
+		free_page((unsigned long)rxs);
 		goto fail;
-	info->tx_ring_ref = err;
-
-	err = xenbus_grant_ring(dev, virt_to_mfn(rxs));
-	if (err < 0)
-		goto fail;
+	}
 	info->rx_ring_ref = err;
 
 	err = xenbus_alloc_evtchn(dev, &info->evtchn);
@@ -374,10 +375,11 @@ static int setup_device(struct xenbus_de
 		goto fail;
 
 	memcpy(netdev->dev_addr, info->mac, ETH_ALEN);
-	info->irq = bind_evtchn_to_irqhandler(
-		info->evtchn, netif_int, SA_SAMPLE_RANDOM, netdev->name,
-		netdev);
-
+	err = bind_evtchn_to_irqhandler(info->evtchn, netif_int,
+					SA_SAMPLE_RANDOM, netdev->name, netdev);
+	if (err < 0)
+		goto fail;
+	info->irq = err;
 	return 0;
 
  fail:
@@ -1397,7 +1399,6 @@ static struct net_device * __devinit cre
 	if (gnttab_alloc_grant_references(RX_MAX_TARGET,
 					  &np->gref_rx_head) < 0) {
 		printk(KERN_ALERT "#### netfront can''t alloc rx grant
refs\n");
-		gnttab_free_grant_references(np->gref_tx_head);
 		err = -ENOMEM;
 		goto exit_free_tx;
 	}

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 8 Jun 2006, at 20:22, Chris Wright wrote:
> There''s a small leak on a couple error paths in setup_device().
> While there rearrange the ring setup order slightly to simplify error
> path since netif_free() will cleanup once ring_ref is valid.  And use
> get_zeroed_page() instead of __get_free_page()/memset().  Handle error 
> if
> bind_evtchn_to_irqhandler() fails, as bad info->irq value is likely to
> cause oops later.  In create_device(), gnttab_free_grant_references()
> is accidentally called twice on tx_head during cleanup from failed
> gnttab_alloc_grant_references() on rx_head, which could corrupt
> gnttab_free_count.
Applied, but I don''t think it''s a complete fix. For example,
shouldn''t
txs/rxs be freed if bind_evtchn_to_irqhandler() fails? There should 
probably be more test-and-free cases on the fail path.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

* Keir Fraser (Keir.Fraser@cl.cam.ac.uk) wrote:
> 
> On 8 Jun 2006, at 20:22, Chris Wright wrote:
> 
> >There''s a small leak on a couple error paths in
setup_device().
> >While there rearrange the ring setup order slightly to simplify error
> >path since netif_free() will cleanup once ring_ref is valid.  And use
> >get_zeroed_page() instead of __get_free_page()/memset().  Handle error 
> >if
> >bind_evtchn_to_irqhandler() fails, as bad info->irq value is likely
to
> >cause oops later.  In create_device(), gnttab_free_grant_references()
> >is accidentally called twice on tx_head during cleanup from failed
> >gnttab_alloc_grant_references() on rx_head, which could corrupt
> >gnttab_free_count.
> 
> Applied, but I don''t think it''s a complete fix. For
example, shouldn''t
> txs/rxs be freed if bind_evtchn_to_irqhandler() fails? There should 
> probably be more test-and-free cases on the fail path.
They are.  netif_free() handles it.  Just not until the ring_ref is
valid (in netif_free()->netif_disconnect_backend()->end_acess()).

Alternative is smth like this (psuedo-patch):

end_access(int ref, void *page)
{
	if (ref != GRANT_INVALID_REF)
		gnttab_end_foreign_access(ref, 0, (unsigned long)page);
+	else
+		if (page)
+			free_page((unsigned long)page);
}

But that seems a bit obtuse to me and possibly error prone, which is
why I chose the method in the patch.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel