Re: [PATCH] net: ethtool: avoid allocation failure for dump_regs

["John W. Linville" <linville@tuxdriver.com>]

I forgot to Cc Johannes and Kalle...

On Thu, Jan 19, 2017 at 09:15:09AM -0500, John W. Linville wrote:
> On Thu, Jan 19, 2017 at 07:35:22AM -0500, David Arcari wrote:
> > On 01/18/2017 11:45 AM, David Miller wrote:
> > > From: David Arcari <darcari@redhat.com>
> > > Date: Wed, 18 Jan 2017 08:34:05 -0500
> > >
> > >> If the user executes 'ethtool -d' for an interface and the associated
> > >> get_regs_len() function returns 0, the user will see a call trace from
> > >> the vmalloc() call in ethtool_get_regs().  This patch modifies
> > >> ethtool_get_regs() to avoid the call to vmalloc when the size is zero.
> > >>
> > >> Signed-off-by: David Arcari <darcari@redhat.com>
> > > I think when the driver indicates this, it is equivalent to saying that
> > > the operation isn't supported.
> > >
> > > Also, this guards us against ->get_regs() methods that don't handle
> > > zero length requests properly.  I see many which are going to do
> > > really terrible things in that situation.
> > >
> > > Therefore, if get_regs_len() returns zero, treat it the safe as if the
> > > ethtool operations were NULL.
> > >
> > > Thanks.
> > 
> > That was actually the fix that I was originally considering, but it
> > turns out
> > there is a problem with it.
> > 
> > I found that the vmalloc error was occurring because
> > ieee80211_get_regs_len() in
> > net/mac80211/ethtool.c was returning zero.  The ieee80211_get_regs in
> > the same
> > file returns the hw version. It turns out that this information is used
> > by the
> > at76c50x-usb driver in the user space ethtool to report which HW variant
> > is in
> > use.  Returning an error when regs_len() returns zero would break this
> > functionality.
> > 
> > -Dave
> 
> I'm responsible for this mess. The original idea was for various
> mac80211-based drivers to override the ethtool operation and provide
> their own dump operation, but the mac80211 crowd never embraced
> the idea.
> 
> In the meantime, I added the default implementation which just
> passed-up wdev->wiphy->hw_version as the version info for a 0-length
> register dump. I then implemented a driver-specific regiser dump
> handler for userland ethtool that would interpret the hardware version
> information for the at76c50x-usb driver.
> 
> So the net of it is, if we treat a return of 0 from get_regs_len()
> as "not supported", we break this one driver-specific feature for
> userland ethtool. Realistically, there are probably very few users
> to care. But I can't guarantee that the number is zero.
> 
> Possible solutions:
> 
> 	-- break userland ethtool for at76c50x-usb
> 	-- avoid 0-len allocation attempt (David Arcari's patch)
> 	-- make allocator accept a 0 length value w/o oops'ing
> 	-- change mac8011 code to return non-zero from get_regs_len()
> 
> Thoughts? The last option holds a certain attraction, but I'm not
> sure how to make it useful...?
> 
> John
> 
> -- 
> John W. Linville		Someday the world will need a hero, and you
> linville@tuxdriver.com			might be all we have.  Be ready.
> 

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.