| Message ID | 20250206235334.1425329-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | de7f7582dff292832fbdeaeff34e6b2ee6f9f95f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | eth: fbnic: support RSS contexts and ntuple filters | expand |
On Thu, Feb 06, 2025 at 03:53:28PM -0800, Jakub Kicinski wrote: > Since commit 42dc431f5d0e ("ethtool: rss: prevent rss ctx deletion > when in use") we prevent removal of RSS contexts pointed to by > existing flow rules. Core should also prevent creation of rules > which point to RSS context which don't exist in the first place. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: ecree.xilinx@gmail.com > CC: gal@nvidia.com > --- > net/ethtool/ioctl.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) Reviewed-by: Joe Damato <jdamato@fastly.com>
On 07/02/2025 1:53, Jakub Kicinski wrote: > Since commit 42dc431f5d0e ("ethtool: rss: prevent rss ctx deletion > when in use") we prevent removal of RSS contexts pointed to by Nit: I would try to avoid the line break in the middle of the cited commit. > existing flow rules. Core should also prevent creation of rules > which point to RSS context which don't exist in the first place. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: ecree.xilinx@gmail.com > CC: gal@nvidia.com > --- > net/ethtool/ioctl.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c > index 7609ce2b2c5e..98b7dcea207a 100644 > --- a/net/ethtool/ioctl.c > +++ b/net/ethtool/ioctl.c > @@ -993,10 +993,14 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev, > return rc; > > /* Nonzero ring with RSS only makes sense if NIC adds them together */ This comment should be moved inside the if statement. > - if (cmd == ETHTOOL_SRXCLSRLINS && info.fs.flow_type & FLOW_RSS && > - !ops->cap_rss_rxnfc_adds && > - ethtool_get_flow_spec_ring(info.fs.ring_cookie)) > - return -EINVAL; > + if (cmd == ETHTOOL_SRXCLSRLINS && info.fs.flow_type & FLOW_RSS) { > + if (!ops->cap_rss_rxnfc_adds && > + ethtool_get_flow_spec_ring(info.fs.ring_cookie)) > + return -EINVAL; > + > + if (!xa_load(&dev->ethtool->rss_ctx, info.rss_context)) Accessing rss_ctx without rss_lock? > + return -EINVAL; > + } > > if (cmd == ETHTOOL_SRXFH && ops->get_rxfh) { > struct ethtool_rxfh_param rxfh = {};
On Sun, 9 Feb 2025 10:17:30 +0200 Gal Pressman wrote: > > /* Nonzero ring with RSS only makes sense if NIC adds them together */ > > This comment should be moved inside the if statement. Will follow up, sorry. > > - if (cmd == ETHTOOL_SRXCLSRLINS && info.fs.flow_type & FLOW_RSS && > > - !ops->cap_rss_rxnfc_adds && > > - ethtool_get_flow_spec_ring(info.fs.ring_cookie)) > > - return -EINVAL; > > + if (cmd == ETHTOOL_SRXCLSRLINS && info.fs.flow_type & FLOW_RSS) { > > + if (!ops->cap_rss_rxnfc_adds && > > + ethtool_get_flow_spec_ring(info.fs.ring_cookie)) > > + return -EINVAL; > > + > > + if (!xa_load(&dev->ethtool->rss_ctx, info.rss_context)) > > Accessing rss_ctx without rss_lock? Yes, same as ethtool_get_max_rxnfc_channel(). Since we'd have to drop the lock instantly after the check the whole rule addition wouldn't be atomic under that lock, anyway. IOW the xa_load() access is safe in itself, and I couldn't think of practical use for taking the rss lock.
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index 7609ce2b2c5e..98b7dcea207a 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -993,10 +993,14 @@ static noinline_for_stack int ethtool_set_rxnfc(struct net_device *dev, return rc; /* Nonzero ring with RSS only makes sense if NIC adds them together */ - if (cmd == ETHTOOL_SRXCLSRLINS && info.fs.flow_type & FLOW_RSS && - !ops->cap_rss_rxnfc_adds && - ethtool_get_flow_spec_ring(info.fs.ring_cookie)) - return -EINVAL; + if (cmd == ETHTOOL_SRXCLSRLINS && info.fs.flow_type & FLOW_RSS) { + if (!ops->cap_rss_rxnfc_adds && + ethtool_get_flow_spec_ring(info.fs.ring_cookie)) + return -EINVAL; + + if (!xa_load(&dev->ethtool->rss_ctx, info.rss_context)) + return -EINVAL; + } if (cmd == ETHTOOL_SRXFH && ops->get_rxfh) { struct ethtool_rxfh_param rxfh = {};
Since commit 42dc431f5d0e ("ethtool: rss: prevent rss ctx deletion when in use") we prevent removal of RSS contexts pointed to by existing flow rules. Core should also prevent creation of rules which point to RSS context which don't exist in the first place. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: ecree.xilinx@gmail.com CC: gal@nvidia.com --- net/ethtool/ioctl.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)