Skip to content

Commit

Permalink
xen-netfront: fix potential deadlock in xennet_remove()
Browse files Browse the repository at this point in the history
[ Upstream commit c2c6331 ]

There's a potential race in xennet_remove(); this is what the driver is
doing upon unregistering a network device:

  1. state = read bus state
  2. if state is not "Closed":
  3.    request to set state to "Closing"
  4.    wait for state to be set to "Closing"
  5.    request to set state to "Closed"
  6.    wait for state to be set to "Closed"

If the state changes to "Closed" immediately after step 1 we are stuck
forever in step 4, because the state will never go back from "Closed" to
"Closing".

Make sure to check also for state == "Closed" in step 4 to prevent the
deadlock.

Also add a 5 sec timeout any time we wait for the bus state to change,
to avoid getting stuck forever in wait_event().

Signed-off-by: Andrea Righi <andrea.righi@canonical.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <sashal@kernel.org>
  • Loading branch information
Andrea Righi authored and Greg Kroah-Hartman committed Aug 5, 2020
1 parent ef47367 commit cb748f1
Showing 1 changed file with 42 additions and 22 deletions.
64 changes: 42 additions & 22 deletions drivers/net/xen-netfront.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ module_param_named(max_queues, xennet_max_queues, uint, 0644);
MODULE_PARM_DESC(max_queues,
"Maximum number of queues per virtual interface");

#define XENNET_TIMEOUT (5 * HZ)

static const struct ethtool_ops xennet_ethtool_ops;

struct netfront_cb {
Expand Down Expand Up @@ -1334,12 +1336,15 @@ static struct net_device *xennet_create_dev(struct xenbus_device *dev)

netif_carrier_off(netdev);

xenbus_switch_state(dev, XenbusStateInitialising);
wait_event(module_wq,
xenbus_read_driver_state(dev->otherend) !=
XenbusStateClosed &&
xenbus_read_driver_state(dev->otherend) !=
XenbusStateUnknown);
do {
xenbus_switch_state(dev, XenbusStateInitialising);
err = wait_event_timeout(module_wq,
xenbus_read_driver_state(dev->otherend) !=
XenbusStateClosed &&
xenbus_read_driver_state(dev->otherend) !=
XenbusStateUnknown, XENNET_TIMEOUT);
} while (!err);

return netdev;

exit:
Expand Down Expand Up @@ -2139,28 +2144,43 @@ static const struct attribute_group xennet_dev_group = {
};
#endif /* CONFIG_SYSFS */

static int xennet_remove(struct xenbus_device *dev)
static void xennet_bus_close(struct xenbus_device *dev)
{
struct netfront_info *info = dev_get_drvdata(&dev->dev);

dev_dbg(&dev->dev, "%s\n", dev->nodename);
int ret;

if (xenbus_read_driver_state(dev->otherend) != XenbusStateClosed) {
if (xenbus_read_driver_state(dev->otherend) == XenbusStateClosed)
return;
do {
xenbus_switch_state(dev, XenbusStateClosing);
wait_event(module_wq,
xenbus_read_driver_state(dev->otherend) ==
XenbusStateClosing ||
xenbus_read_driver_state(dev->otherend) ==
XenbusStateUnknown);
ret = wait_event_timeout(module_wq,
xenbus_read_driver_state(dev->otherend) ==
XenbusStateClosing ||
xenbus_read_driver_state(dev->otherend) ==
XenbusStateClosed ||
xenbus_read_driver_state(dev->otherend) ==
XenbusStateUnknown,
XENNET_TIMEOUT);
} while (!ret);

if (xenbus_read_driver_state(dev->otherend) == XenbusStateClosed)
return;

do {
xenbus_switch_state(dev, XenbusStateClosed);
wait_event(module_wq,
xenbus_read_driver_state(dev->otherend) ==
XenbusStateClosed ||
xenbus_read_driver_state(dev->otherend) ==
XenbusStateUnknown);
}
ret = wait_event_timeout(module_wq,
xenbus_read_driver_state(dev->otherend) ==
XenbusStateClosed ||
xenbus_read_driver_state(dev->otherend) ==
XenbusStateUnknown,
XENNET_TIMEOUT);
} while (!ret);
}

static int xennet_remove(struct xenbus_device *dev)
{
struct netfront_info *info = dev_get_drvdata(&dev->dev);

xennet_bus_close(dev);
xennet_disconnect_backend(info);

if (info->netdev->reg_state == NETREG_REGISTERED)
Expand Down

0 comments on commit cb748f1

Please sign in to comment.