Re: [net 8/8] ixgbe: ethtool: stats user buffer overrun

[Ben Hutchings <bhutchings@solarflare.com>]

On Wed, 2012-02-08 at 01:36 -0800, Jeff Kirsher wrote:
> From: John Fastabend <john.r.fastabend@intel.com>
> 
> If the number of tx/rx queues changes the ethtool ioctl
> ETHTOOL_GSTATS may overrun the userspace buffer. This
> occurs because the general practice in user space to
> query stats is to issue a ETHTOOL_GSSET cmd to learn the
> buffer size needed, allocate the buffer, then call
> ETHTOOL_GSTIRNGS and ETHTOOL_GSTATS. If the number of
> real_num_queues is changed or flow control attributes
> are changed after ETHTOOL_GSSET but before the
> ETHTOOL_GSTRINGS/ETHTOOL_GSTATS a user space buffer
> overrun occurs.

This is a problem with several ethtool operations - the user buffer size
is implicit.

> To fix the overrun always return the max buffer size
> needed from get_sset_count() then return all strings
> and stats from get_strings()/get_ethtool_stats().
> 
> This _will_ change the output from the ioctl() call
> which could break applications and script parsing in
> theory. I believe these changes should not break existing
> tools because the only changes will be more {tx|rx}_queues
> and the {tx|rx}_pb_* stats will always be returned.
[...]

Yes, this sounds perfectly reasonable.  And this change is definitely
worth making.

However, even if the number of stats (or other attributes) for a device
never change at run-time, devices can be renamed concurrently so that
the second operation runs on a different device!

Perhaps we could change dev_ioctl so that if ifreq::ifr_name is an empty
string then the socket's bound device is used.  However, setting
SO_BINDTODEVICE currently requires CAP_NET_RAW.  Alternately, there
could be some sort of union between ifr_name and an ifindex.

Ben.

-- 
Ben Hutchings, Staff 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.