netdev - Re: ethtool physical identify vs netlink locking?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1301443676.10056.57.camel@localhost>
Date:	Wed, 30 Mar 2011 01:07:56 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Stephen Hemminger <shemminger@...tta.com>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: ethtool physical identify vs netlink locking?

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html