Right now if an administrator uses the ethtool function to identify network
interface, the netlink lock can be held indefinitely. In other words, doing
"ethtool -p eth1" will stop all other netlink activity. This is bad, imagine
the case of an operator doing that to find a NIC in a rack, and because of
the netlink lockout all routing daemon activity stops.
There are several possible solutions but most involve fixing all the device
drivers (24). Options:
1. Have device driver drop and reacquire rtnl() while blinking
2. Have ethtool core drop rtnl before calling device driver
3. Add per-device ethtool rtnl lock
#1 is the least disruption
#2 means additional locking maybe required for each device driver
#3 seems like excessive overhead.
Comments?
On Tue, 2011-03-29 at 13:52 -0700, Stephen Hemminger wrote:
Right now if an administrator uses the ethtool function to identify network
interface, the netlink lock can be held indefinitely. In other words, doing
"ethtool -p eth1" will stop all other netlink activity. This is bad, imagine
the case of an operator doing that to find a NIC in a rack, and because of
the netlink lockout all routing daemon activity stops.
Also, glibc can enumerate devices during name lookup now (if I remember
correctly), so new connections to servers that do reverse name lookups
tend to stall immediately.
There are several possible solutions but most involve fixing all the device
drivers (24). Options:
1. Have device driver drop and reacquire rtnl() while blinking
2. Have ethtool core drop rtnl before calling device driver
3. Add per-device ethtool rtnl lock
#1 is the least disruption
but nasty!
#2 means additional locking maybe required for each device driver
#3 seems like excessive overhead.
In the sfc driver, physical ID used to be delegated to the PHY
operations. Then I realised that it was pointless to use a PHY's blink
mode where it was available and a periodic timer on the host where it
wasn't, when the latter would work for all of them. So I would propose:
4. Define a ethtool operation 'set_id_state' with an argument that sets
identification on/off/inactive/active (the last optional, for any driver
that really wants to do this differently). When this is defined, the
ethtool core runs the loop and acquires the lock each time it calls this
operation.
This requires changes to every driver, though not all at once. As an
additional benefit, it should result in consistent behaviour for the
count = 0 case.
The core ethtool function would look something like:
static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
{
struct ethtool_value id;
int rc;
if (!dev->ethtool_ops->phys_id && !dev->ethtool_ops->set_id_led)
return -EOPNOTSUPP;
if (copy_from_user(&id, useraddr, sizeof(id)))
return -EFAULT;
if (!dev->ethtool_ops->set_id_led)
/* Do it the old way */
return dev->ethtool_ops->phys_id(dev, id.data);
rc = dev->ethtool_ops->set_id_state(dev, ETHTOOL_ID_ACTIVE);
if (rc && rc != -EINVAL)
return rc;
dev_hold(dev);
rtnl_unlock();
if (rc == 0) {
/* Driver will handle this itself */
schedule_timeout_interruptible(
id.data ? id.data : MAX_SCHEDULE_TIMEOUT);
} else {
/* Driver expects to be called periodically */
do {
rtnl_lock();
rc = dev->ethtool_ops->set_id_state(dev, ETHTOOL_ID_ON);
rtnl_unlock();
if (rc)
break;
schedule_timeout_interruptible(HZ / 2);
rtnl_lock();
rc = dev->ethtool_ops->set_id_state(dev, ETHTOOL_ID_OFF);
rtnl_unlock();
if (rc)
break;
schedule_timeout_interruptible(HZ / 2);
} while (!signal_pending(current) &&
(id.data == 0 || --id.data != 0));
}
rtnl_lock();
dev_put(dev);
(void)dev->ethtool_ops->set_id_state(dev, ETHTOOL_ID_INACTIVE);
return rc;
}
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
From: Ben Hutchings <redacted>
Date: Wed, 30 Mar 2011 01:07:56 +0100
In the sfc driver, physical ID used to be delegated to the PHY
operations. Then I realised that it was pointless to use a PHY's blink
mode where it was available and a periodic timer on the host where it
wasn't, when the latter would work for all of them. So I would propose:
4. Define a ethtool operation 'set_id_state' with an argument that sets
identification on/off/inactive/active (the last optional, for any driver
that really wants to do this differently). When this is defined, the
ethtool core runs the loop and acquires the lock each time it calls this
operation.
This requires changes to every driver, though not all at once. As an
additional benefit, it should result in consistent behaviour for the
count = 0 case.
This seems like a good way to solve the problem.
On Wed, 30 Mar 2011 01:07:56 +0100
Ben Hutchings [off-list ref] wrote:
4. Define a ethtool operation 'set_id_state' with an argument that sets
I like this way, but the code should return -EINTR if interrupted?
2011/3/30 Ben Hutchings [off-list ref]:
On Tue, 2011-03-29 at 13:52 -0700, Stephen Hemminger wrote:
quoted
Right now if an administrator uses the ethtool function to identify network
interface, the netlink lock can be held indefinitely. In other words, doing
"ethtool -p eth1" will stop all other netlink activity. This is bad, imagine
the case of an operator doing that to find a NIC in a rack, and because of
the netlink lockout all routing daemon activity stops.
[...]
quoted
There are several possible solutions but most involve fixing all the device
drivers (24). Options:
1. Have device driver drop and reacquire rtnl() while blinking
2. Have ethtool core drop rtnl before calling device driver
3. Add per-device ethtool rtnl lock
4. Define a ethtool operation 'set_id_state' with an argument that sets
identification on/off/inactive/active (the last optional, for any driver
that really wants to do this differently). When this is defined, the
ethtool core runs the loop and acquires the lock each time it calls this
operation.
5. Have a driver register a LED class device instead of implementing
an ethtool op.
Hmm. This would require changes to userspace ethtool command. I wonder
if anything else uses this call?
Best Regards,
Michał Mirosław
On Wed, 30 Mar 2011 03:35:40 +0200
Michał Mirosław [off-list ref] wrote:
2011/3/30 Ben Hutchings [off-list ref]:
quoted
On Tue, 2011-03-29 at 13:52 -0700, Stephen Hemminger wrote:
quoted
Right now if an administrator uses the ethtool function to identify network
interface, the netlink lock can be held indefinitely. In other words, doing
"ethtool -p eth1" will stop all other netlink activity. This is bad, imagine
the case of an operator doing that to find a NIC in a rack, and because of
the netlink lockout all routing daemon activity stops.
[...]
quoted
quoted
There are several possible solutions but most involve fixing all the device
drivers (24). Options:
1. Have device driver drop and reacquire rtnl() while blinking
2. Have ethtool core drop rtnl before calling device driver
3. Add per-device ethtool rtnl lock
4. Define a ethtool operation 'set_id_state' with an argument that sets
identification on/off/inactive/active (the last optional, for any driver
that really wants to do this differently). When this is defined, the
ethtool core runs the loop and acquires the lock each time it calls this
operation.
5. Have a driver register a LED class device instead of implementing
an ethtool op.
Hmm. This would require changes to userspace ethtool command. I wonder
if anything else uses this call?
Full LED support is overkill for this I think. Especially if it means creating
24 unique LED drivers.
Ben's idea seams the best so far.