Re: [PATCHv3 net-next-2.6 2/3] qlcnic: Take FW dump via ethtool

[Ben Hutchings <bhutchings@solarflare.com>]

On Wed, 2011-05-11 at 15:54 -0700, Anirban Chakraborty wrote:
> Driver checks if the previous dump has been cleared before taking the dump.
> It doesn't take the dump if it is not cleared.
> 
> Changes from v2:
> Added lock to protect dump data structures from being mangled while
> dumping or setting them via ethtool.

Unfortunately it still seems to be possible for the dump length to
change between the ethtool core calling qlcnic_get_dump_flag() and
qlcnic_get_dump_data().

So I think qlcnic_get_dump_data() will need to double-check the length
after taking the internal lock:

[...]
> +static int
> +qlcnic_get_dump_data(struct net_device *netdev, struct ethtool_dump *dump,
> +                       void *buffer)
> +{
> +       int i, copy_sz;
> +       u32 *hdr_ptr, *data;
> +       struct qlcnic_adapter *adapter = netdev_priv(netdev);
> +       struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump;
> +
> +       if (qlcnic_api_lock(adapter))
> +               return -EIO;
[...]

	if (dump->len < fw_dump->tmpl_hdr->size + fw_dump->size) {
		qlcnic_api_unlock(adapter);
		return -EINVAL;
	}

I'm not sure about the error code... and I'm really not happy about the
need to check lengths in both the ethtool core and the driver.

Can't you change the function that actually makes a dump to acquire the
RTNL lock?  (You'll need to do that *before* acquiring the driver's own
lock.)

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.