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

On Wed, 2012-02-08 at 01:36 -0800, Jeff Kirsher wrote:

From: John Fastabend <redacted>

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.