Skip to contentSkip to conversation

netfront: plug a theoretical leak on setup_device()'s error path

PATCH New Laszlo Ersek 1 participant 1 message May 24, 2011 Xen hypervisor interface

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 netfront: plug a theoretical leak on setup_device()'s error path

This patch intends to fix a theoretical leak in
netfront::setup_device(), on the error path.

Suppose the frontend is in XenbusStateInitialising and the backend advances to XenbusStateInitWait. backend_changed() will call network_connect() -> talk_to_backend() -> setup_device().

If bind_listening_port_to_irqhandler() fails (due to event channel allocation failure, dynamic IRQ allocation failure etc), then the grant references and the shared ring pages remain allocated. The error percolates back to backend_changed() without any cleanup. backend_changed() will not change the frontend state; it will stay in XenbusStateInitialising. If the backend retries XenbusStateInitWait (possibly through a fake, no-op mediate state, like XenbusStateClosed), then the frontend repeats the above call chain and leaks the previously unreleased ring pages and grant references.

If setup_device() fails to grant access to the RX page before it tries to call bind_listening_port_to_irqhandler(), then only the TX page / grant reference are leaked in the next round.

I'm not sure what happens when the RX page allocation fails:
xenbus_dev_fatal() is called then, which changes the frontend's state and seems to preclude a second immediate call to network_connect(). I think the patch below shouldn't hurt in that case either (or after earlier failures in setup_device() for that matter).

Thanks for considering.

---
 drivers/xen/netfront/netfront.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
drivers/xen/netfront/netfront.c +12 -6
diff -r 415a9b435fef drivers/xen/netfront/netfront.c
@@ -215,6 +215,7 @@ static int setup_device(struct xenbus_de
 static struct net_device *create_netdev(struct xenbus_device *);
 
 static void end_access(int, void *);
+static void netif_release_rings(struct netfront_info *);
 static void netif_disconnect_backend(struct netfront_info *);
 
 static int network_connect(struct net_device *);
@@ -522,6 +523,7 @@ static int setup_device(struct xenbus_de
 	return 0;
 
  fail:
+	netif_release_rings(info);
 	return err;
 }
 
@@ -2152,6 +2154,15 @@ static struct notifier_block notifier_in
 };
 #endif
 
+static void netif_release_rings(struct netfront_info *info)
+{
+	end_access(info->tx_ring_ref, info->tx.sring);
+	end_access(info->rx_ring_ref, info->rx.sring);
+	info->tx_ring_ref = GRANT_INVALID_REF;
+	info->rx_ring_ref = GRANT_INVALID_REF;
+	info->tx.sring = NULL;
+	info->rx.sring = NULL;
+}
 
 static void netif_disconnect_backend(struct netfront_info *info)
 {
@@ -2166,12 +2177,7 @@ static void netif_disconnect_backend(str
 		unbind_from_irqhandler(info->irq, info->netdev);
 	info->irq = 0;
 
-	end_access(info->tx_ring_ref, info->tx.sring);
-	end_access(info->rx_ring_ref, info->rx.sring);
-	info->tx_ring_ref = GRANT_INVALID_REF;
-	info->rx_ring_ref = GRANT_INVALID_REF;
-	info->tx.sring = NULL;
-	info->rx.sring = NULL;
+	netif_release_rings(info);
 }