| Message ID | 1324084683.2798.45.camel@bwh-desktop |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index b38bf69..1c003d4 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -501,10 +501,23 @@ struct ethtool_rx_flow_spec { * must use the second parameter to get_rxnfc() instead of @rule_locs. * * For %ETHTOOL_SRXCLSRLINS, @fs specifies the rule to add or update. - * @fs.@location specifies the location to use and must not be ignored. + * @fs.@location either specifies the location to use or is a special + * location value with %RX_CLS_LOC_SPECIAL flag set. Drivers do not + * have to support special location values but must return -%EINVAL if + * passed an unsupported value. On return, @fs.@location is the + * actual rule location. * * For %ETHTOOL_SRXCLSRLDEL, @fs.@location specifies the location of an * existing rule on entry. + * + * A driver supporting the special location values for + * %ETHTOOL_SRXCLSRLINS may add the rule at any suitable unused + * location, and may remove a rule at a later location (lower + * priority) that matches exactly the same set of flows. The special + * values are: %RX_CLS_LOC_ANY, selecting any location; + * %RX_CLS_LOC_FIRST, selecting the first suitable location (maximum + * priority); and %RX_CLS_LOC_LAST, selecting the last suitable + * location (minimum priority). */ struct ethtool_rxnfc { __u32 cmd; @@ -1141,6 +1154,12 @@ struct ethtool_ops { #define RX_CLS_FLOW_DISC 0xffffffffffffffffULL +/* Special RX classification rule insert location values */ +#define RX_CLS_LOC_SPECIAL 0x80000000 /* flag */ +#define RX_CLS_LOC_ANY 0xffffffff +#define RX_CLS_LOC_FIRST 0xfffffffe +#define RX_CLS_LOC_LAST 0xfffffffd + /* Reset flags */ /* The reset() operation must clear the flags for the components which * were actually reset. On successful return, the flags indicate the diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 597732c..e88b80d 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -439,6 +439,7 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev, { struct ethtool_rxnfc info; size_t info_size = sizeof(info); + int rc; if (!dev->ethtool_ops->set_rxnfc) return -EOPNOTSUPP; @@ -454,7 +455,15 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev, if (copy_from_user(&info, useraddr, info_size)) return -EFAULT; - 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; + + return 0; } static noinline_for_stack int ethtool_get_rxnfc(struct net_device *dev, diff --git a/net/socket.c b/net/socket.c index e62b4f0..2cad581 100644 --- a/net/socket.c +++ b/net/socket.c @@ -2758,10 +2758,10 @@ static int ethtool_ioctl(struct net *net, struct compat_ifreq __user *ifr32) case ETHTOOL_GRXRINGS: case ETHTOOL_GRXCLSRLCNT: case ETHTOOL_GRXCLSRULE: + case ETHTOOL_SRXCLSRLINS: convert_out = true; /* fall through */ case ETHTOOL_SRXCLSRLDEL: - case ETHTOOL_SRXCLSRLINS: buf_size += sizeof(struct ethtool_rxnfc); convert_in = true; break;
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> --- This change is sufficient to allow for conversion of sfc to implementing the RX NFC operations. I don't know whether anything more would be necessary for other hardware/drivers. The ethtool_ops::set_rx_ntuple operation would then be removed, as there will be no in-tree implementations. In order for userland to make use of the special location values and reliably fallback to userland allocation, drivers must properly range-check location values. gianfar wasn't doing this, and as a result it also didn't properly limit the total number of rules. I hope the fix for that can go into stable updates and userland can then ignore the old versions of gianfar. I'll post the ethtool changes as a follow-up. If anyone would like to see the changes to sfc at this stage, I can post them too. Ben. include/linux/ethtool.h | 21 ++++++++++++++++++++- net/core/ethtool.c | 11 ++++++++++- net/socket.c | 2 +- 3 files changed, 31 insertions(+), 3 deletions(-)