| Message ID | 20240702164157.4018425-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1a16cdf77e0d7de0fb640e65d65c0898b38c1b4b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethtool: fix compat with old RSS context API | expand |
On 02/07/2024 17:41, Jakub Kicinski wrote: > Device driver gets access to rxfh_dev, while rxfh is just a local > copy of user space params. We need to check what RSS context ID > driver assigned in rxfh_dev, not rxfh. > > Using rxfh leads to trying to store all contexts at index 0xffffffff. > From the user perspective it leads to "driver chose duplicate ID" > warnings when second context is added and inability to access any > contexts even tho they were successfully created - xa_load() for > the actual context ID will return NULL, and syscall will return -ENOENT. > > Looks like a rebasing mistake, since rxfh_dev was added relatively > recently by fb6e30a72539 ("net: ethtool: pass a pointer to parameters > to get/set_rxfh ethtool ops"). > > Fixes: eac9122f0c41 ("net: ethtool: record custom RSS contexts in the XArray") > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Reviewed-by: Edward Cree <ecree.xilinx@gmail.com> My bad, I should have re-tested that side of things after the rebase onto Ahmed's changes, instead of just testing the new API via sfc. Thanks for catching it so quickly.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 2 Jul 2024 09:41:57 -0700 you wrote: > Device driver gets access to rxfh_dev, while rxfh is just a local > copy of user space params. We need to check what RSS context ID > driver assigned in rxfh_dev, not rxfh. > > Using rxfh leads to trying to store all contexts at index 0xffffffff. > From the user perspective it leads to "driver chose duplicate ID" > warnings when second context is added and inability to access any > contexts even tho they were successfully created - xa_load() for > the actual context ID will return NULL, and syscall will return -ENOENT. > > [...] Here is the summary with links: - [net-next] net: ethtool: fix compat with old RSS context API https://git.kernel.org/netdev/net-next/c/1a16cdf77e0d You are awesome, thank you!
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c index d8795ed07ba3..46f0497ae6bc 100644 --- a/net/ethtool/ioctl.c +++ b/net/ethtool/ioctl.c @@ -1483,13 +1483,13 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev, /* Update rss_ctx tracking */ if (create && !ops->create_rxfh_context) { /* driver uses old API, it chose context ID */ - if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh.rss_context))) { + if (WARN_ON(xa_load(&dev->ethtool->rss_ctx, rxfh_dev.rss_context))) { /* context ID reused, our tracking is screwed */ kfree(ctx); goto out; } /* Allocate the exact ID the driver gave us */ - if (xa_is_err(xa_store(&dev->ethtool->rss_ctx, rxfh.rss_context, + if (xa_is_err(xa_store(&dev->ethtool->rss_ctx, rxfh_dev.rss_context, ctx, GFP_KERNEL))) { kfree(ctx); goto out;
Device driver gets access to rxfh_dev, while rxfh is just a local copy of user space params. We need to check what RSS context ID driver assigned in rxfh_dev, not rxfh. Using rxfh leads to trying to store all contexts at index 0xffffffff. From the user perspective it leads to "driver chose duplicate ID" warnings when second context is added and inability to access any contexts even tho they were successfully created - xa_load() for the actual context ID will return NULL, and syscall will return -ENOENT. Looks like a rebasing mistake, since rxfh_dev was added relatively recently by fb6e30a72539 ("net: ethtool: pass a pointer to parameters to get/set_rxfh ethtool ops"). Fixes: eac9122f0c41 ("net: ethtool: record custom RSS contexts in the XArray") Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: przemyslaw.kitszel@intel.com CC: andrew@lunn.ch CC: ecree.xilinx@gmail.com CC: ahmed.zaki@intel.com --- net/ethtool/ioctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)