| Message ID | 20240807132541.3460386-1-gal@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ethtool: Fix context creation with no parameters | expand |
At a glance I think you popped it into the wrong place. On Wed, 7 Aug 2024 16:25:41 +0300 Gal Pressman wrote: > - if ((rxfh.indir_size && > + if (!create && ((rxfh.indir_size && > rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE && > rxfh.indir_size != dev_indir_size) || This condition just checks if indir_size matches the device expectations, is reset or is zero. Even if we're creating the context, at present, the indir table size is fixed for the device. > (rxfh.key_size && (rxfh.key_size != dev_key_size)) || similarly this checks the key size > (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE && > rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE && > - rxfh.input_xfrm == RXH_XFRM_NO_CHANGE)) > + rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))) only this validates the "is this a nop", so you gotta add the && !create here That's why I (perhaps not very clearly) suggested that we should split this if into two. Cause the first two conditions check "sizes" while the last chunk checks for "nop". And readability suffers. Feel free to send the new version tomorrow without a full 24h wait.
On 07/08/2024 17:21, Jakub Kicinski wrote: > At a glance I think you popped it into the wrong place. > > On Wed, 7 Aug 2024 16:25:41 +0300 Gal Pressman wrote: >> - if ((rxfh.indir_size && >> + if (!create && ((rxfh.indir_size && >> rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE && >> rxfh.indir_size != dev_indir_size) || > > This condition just checks if indir_size matches the device > expectations, is reset or is zero. Even if we're creating the > context, at present, the indir table size is fixed for the device. > >> (rxfh.key_size && (rxfh.key_size != dev_key_size)) || > > similarly this checks the key size > >> (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE && >> rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE && >> - rxfh.input_xfrm == RXH_XFRM_NO_CHANGE)) >> + rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))) > > only this validates the "is this a nop", so you gotta add the && > !create here Indeed, I was too concentrated on shutting up this check. > > That's why I (perhaps not very clearly) suggested that we should split > this if into two. Cause the first two conditions check "sizes" while > the last chunk checks for "nop". And readability suffers. You were clear, I was hoping to split it into steps/patches. > > Feel free to send the new version tomorrow without a full 24h wait. Thanks for the review, I'll try to send it tomorrow.
On 07/08/2024 16:25, Gal Pressman wrote: > diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c > index 8ca13208d240..2fdbdcfa1506 100644 > --- a/net/ethtool/ioctl.c > +++ b/net/ethtool/ioctl.c > @@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, > /* If either indir, hash key or function is valid, proceed further. This comment is wrong, the check doesn't really verify the hash function is valid. I'll remove it in my fix unless you have any objections. Not verifying the hash function is probably another bug, but it's too late to fix it at this point? > * Must request at least one change: indir size, hash key, function > * or input transformation. > + * There's no need for any of it in case of context creation. > */
On Wed, 7 Aug 2024 19:14:57 +0300 Gal Pressman wrote: > > @@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, > > /* If either indir, hash key or function is valid, proceed further. > > This comment is wrong, the check doesn't really verify the hash function > is valid. I'll remove it in my fix unless you have any objections. > > Not verifying the hash function is probably another bug, but it's too > late to fix it at this point? Let's do that separately in net-next? Old ethtool had a bit of a "forward compatibility" mindset, so it may have been intentional.
On 07/08/2024 19:14, Gal Pressman wrote: > On 07/08/2024 16:25, Gal Pressman wrote: >> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c >> index 8ca13208d240..2fdbdcfa1506 100644 >> --- a/net/ethtool/ioctl.c >> +++ b/net/ethtool/ioctl.c >> @@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, >> /* If either indir, hash key or function is valid, proceed further. > > This comment is wrong, the check doesn't really verify the hash function > is valid. I'll remove it in my fix unless you have any objections. Either this comment is horribly wrong or it's too late for me to look at code.. The code doesn't proceed if either indir or key are valid, it returns if either of them are invalid, I'm removing this comment altogether..
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 8ca13208d240..2fdbdcfa1506 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1372,14 +1372,15 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, /* If either indir, hash key or function is valid, proceed further. * Must request at least one change: indir size, hash key, function * or input transformation. + * There's no need for any of it in case of context creation. */ - if ((rxfh.indir_size && + if (!create && ((rxfh.indir_size && rxfh.indir_size != ETH_RXFH_INDIR_NO_CHANGE && rxfh.indir_size != dev_indir_size) || (rxfh.key_size && (rxfh.key_size != dev_key_size)) || (rxfh.indir_size == ETH_RXFH_INDIR_NO_CHANGE && rxfh.key_size == 0 && rxfh.hfunc == ETH_RSS_HASH_NO_CHANGE && - rxfh.input_xfrm == RXH_XFRM_NO_CHANGE)) + rxfh.input_xfrm == RXH_XFRM_NO_CHANGE))) return -EINVAL; indir_bytes = dev_indir_size * sizeof(rxfh_dev.indir[0]);
The 'at least one change' requirement is not applicable for context creation, skip the check in such case. This allows a command such as 'ethtool -X eth0 context new' to work. The command works by mistake when using older versions of userspace ethtool due to an incompatibility issue where rxfh.input_xfrm is passed as zero (unset) instead of RXH_XFRM_NO_CHANGE as done with recent userspace. This patch does not try to solve the incompatibility issue. Link: https://lore.kernel.org/netdev/05ae8316-d3aa-4356-98c6-55ed4253c8a7@nvidia.com/ Fixes: 84a1d9c48200 ("net: ethtool: extend RXNFC API to support RSS spreading of filter matches") Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com> Reviewed-by: Jianbo Liu <jianbol@nvidia.com> Signed-off-by: Gal Pressman <gal@nvidia.com> --- net/ethtool/ioctl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)