Re: [RFC][PATCH net-next] ethtool: Allow drivers to select RX NFC rule locations

[Ben Hutchings <bhutchings@solarflare.com>]

On Fri, 2011-12-16 at 18:44 -0800, Dimitris Michailidis wrote:
> On 12/16/2011 05:18 PM, Ben Hutchings wrote:
> > Define special location values for RX NFC that request the driver to
> > select the actual rule location.  This allows for implementation on
> > devices that use hash-based filter lookup, whereas currently the API is
> > more suited to devices with TCAM lookup or linear search.
> > 
> > In ethtool_set_rxnfc() and the compat wrapper ethtool_ioctl(), copy
> > the structure back to user-space after insertion so that the actual
> > location is returned.
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> 
> I like this change.  One concern below.
> 
> > -	return dev->ethtool_ops->set_rxnfc(dev, &info);
> > +	rc = dev->ethtool_ops->set_rxnfc(dev, &info);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (cmd == ETHTOOL_SRXCLSRLINS &&
> > +	    copy_to_user(useraddr, &info, info_size))
> > +		return -EFAULT;
> 
> Here we return failure but the rule has been added successfully and is in 
> effect.  It may be better to return 0 and let user-space tell this last step 
> failed by the fact that the location field is still special.

If copy_to_user() fails then the user program has a bug (or is probing
for security flaws).  A return value of -EFAULT is morally equivalent to
SIGSEGV.  (I'm not sure why it isn't translated into a signal on return,
but I imagine there are historical reasons.)  I don't see any point in
trying to help userland recover from this.

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.