[PATCH net-next 0/2] ethtool: rss: track rss ctx busy from core

[PATCH net-next 0/2] ethtool: rss: track rss ctx busy from core

[Daniel Zahka]

This series prevents deletion of rss contexts that are
in use by ntuple filters from ethtool core.

Daniel Zahka (2):
  ethtool: rss: prevent rss ctx deletion when in use
  selftests: drv-net: rss_ctx: add rss ctx busy testcase

 net/ethtool/common.c                          | 48 +++++++++++++++++++
 net/ethtool/common.h                          |  1 +
 net/ethtool/ioctl.c                           |  7 +++
 .../selftests/drivers/net/hw/rss_ctx.py       | 32 ++++++++++++-
 4 files changed, 86 insertions(+), 2 deletions(-)

-- 
2.43.5

[PATCH net-next 1/2] ethtool: rss: prevent rss ctx deletion when in use

[Daniel Zahka]

ntuple filters can specify an rss context to use for packet hashing
and queue selection. When a filter is referencing an rss context, it
should be invalid for that context to be deleted. A list of active
ntuple filters and their associated rss contexts can be compiled by
querying a device's ethtool_ops.get_rxnfc. This patch checks to see if
any ntuple filters are referencing an rss context during context
deletion, and prevents the deletion if the requested context is still
in use.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 net/ethtool/common.c | 48 ++++++++++++++++++++++++++++++++++++++++++++
 net/ethtool/common.h |  1 +
 net/ethtool/ioctl.c  |  7 +++++++
 3 files changed, 56 insertions(+)

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index dd345efa114b..0d62363dbd9d 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -684,6 +684,54 @@ int ethtool_check_max_channel(struct net_device *dev,
 	return 0;
 }
 
+int ethtool_check_rss_ctx_busy(struct net_device *dev, u32 rss_context)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_rxnfc *info;
+	int rc, i, rule_cnt;
+
+	if (!ops->get_rxnfc)
+		return 0;
+
+	rule_cnt = ethtool_get_rxnfc_rule_count(dev);
+	if (!rule_cnt)
+		return 0;
+
+	if (rule_cnt < 0)
+		return -EINVAL;
+
+	info = kvzalloc(struct_size(info, rule_locs, rule_cnt), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->cmd = ETHTOOL_GRXCLSRLALL;
+	info->rule_cnt = rule_cnt;
+	rc = ops->get_rxnfc(dev, info, info->rule_locs);
+	if (rc)
+		goto out_free;
+
+	for (i = 0; i < rule_cnt; i++) {
+		struct ethtool_rxnfc rule_info = {
+			.cmd = ETHTOOL_GRXCLSRULE,
+			.fs.location = info->rule_locs[i],
+		};
+
+		rc = ops->get_rxnfc(dev, &rule_info, NULL);
+		if (rc)
+			goto out_free;
+
+		if (rule_info.fs.flow_type & FLOW_RSS &&
+		    rule_info.rss_context == rss_context) {
+			rc = -EBUSY;
+			goto out_free;
+		}
+	}
+
+out_free:
+	kvfree(info);
+	return rc;
+}
+
 int ethtool_check_ops(const struct ethtool_ops *ops)
 {
 	if (WARN_ON(ops->set_coalesce && !ops->supported_coalesce_params))
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index d55d5201b085..4a2de3ce7354 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -47,6 +47,7 @@ bool convert_legacy_settings_to_link_ksettings(
 int ethtool_check_max_channel(struct net_device *dev,
 			      struct ethtool_channels channels,
 			      struct genl_info *info);
+int ethtool_check_rss_ctx_busy(struct net_device *dev, u32 rss_context);
 int __ethtool_get_ts_info(struct net_device *dev, struct kernel_ethtool_ts_info *info);
 
 extern const struct ethtool_phy_ops *ethtool_phy_ops;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 04b34dc6b369..5cc131cdb1bc 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1462,6 +1462,13 @@ static noinline_for_stack int ethtool_set_rxfh(struct net_device *dev,
 		mutex_lock(&dev->ethtool->rss_lock);
 		locked = true;
 	}
+
+	if (rxfh.rss_context && rxfh_dev.rss_delete) {
+		ret = ethtool_check_rss_ctx_busy(dev, rxfh.rss_context);
+		if (ret)
+			goto out;
+	}
+
 	if (create) {
 		if (rxfh_dev.rss_delete) {
 			ret = -EINVAL;
-- 
2.43.5

[PATCH net-next 2/2] selftests: drv-net: rss_ctx: add rss ctx busy testcase

[Daniel Zahka]

It should be invalid to delete an rss context while it is being
referenced from an ntuple filter. ethtool core should prevent this
from happening. This patch adds a testcase to verify this behavior.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 .../selftests/drivers/net/hw/rss_ctx.py       | 32 +++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
index 9d7adb3cf33b..29995586993c 100755
--- a/tools/testing/selftests/drivers/net/hw/rss_ctx.py
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -6,7 +6,7 @@ import random
 from lib.py import ksft_run, ksft_pr, ksft_exit, ksft_eq, ksft_ne, ksft_ge, ksft_lt
 from lib.py import NetDrvEpEnv
 from lib.py import EthtoolFamily, NetdevFamily
-from lib.py import KsftSkipEx
+from lib.py import KsftSkipEx, KsftFailEx
 from lib.py import rand_port
 from lib.py import ethtool, ip, defer, GenerateTraffic, CmdExitFailure
 
@@ -606,6 +606,33 @@ def test_rss_context_overlap2(cfg):
     test_rss_context_overlap(cfg, True)
 
 
+def test_delete_rss_context_busy(cfg):
+    """
+    Test that deletion returns -EBUSY when an rss context is being used
+    by an ntuple filter.
+    """
+
+    require_ntuple(cfg)
+
+    # create additional rss context
+    ctx_id = ethtool_create(cfg, "-X", "context new")
+    ctx_deleter = defer(ethtool, f"-X {cfg.ifname} context {ctx_id} delete")
+
+    # utilize context from ntuple filter
+    port = rand_port()
+    flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {ctx_id}"
+    ntuple_id = ethtool_create(cfg, "-N", flow)
+    defer(ethtool, f"-N {cfg.ifname} delete {ntuple_id}")
+
+    # attempt to delete in-use context
+    try:
+        ctx_deleter.exec_only()
+        ctx_deleter.cancel()
+        raise KsftFailEx(f"deleted context {ctx_id} used by rule {ntuple_id}")
+    except CmdExitFailure:
+        pass
+
+
 def main() -> None:
     with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
         cfg.ethnl = EthtoolFamily()
@@ -616,7 +643,8 @@ def main() -> None:
                   test_rss_context, test_rss_context4, test_rss_context32,
                   test_rss_context_dump, test_rss_context_queue_reconfigure,
                   test_rss_context_overlap, test_rss_context_overlap2,
-                  test_rss_context_out_of_order, test_rss_context4_create_with_cfg],
+                  test_rss_context_out_of_order, test_rss_context4_create_with_cfg,
+                  test_delete_rss_context_busy],
                  args=(cfg, ))
     ksft_exit()
 
-- 
2.43.5

Re: [PATCH net-next 1/2] ethtool: rss: prevent rss ctx deletion when in use

[Edward Cree]

On 11/10/2024 19:35, Daniel Zahka wrote:
> A list of active
> ntuple filters and their associated rss contexts can be compiled by
> querying a device's ethtool_ops.get_rxnfc. This patch checks to see if
> any ntuple filters are referencing an rss context during context
> deletion, and prevents the deletion if the requested context is still
> in use.

Imho it would make more sense to add core tracking of ntuple
 filters, along with a refcount on the rss context.  That way
 context deletion just has to check the count is zero.

-ed

Re: [PATCH net-next 1/2] ethtool: rss: prevent rss ctx deletion when in use

[Jakub Kicinski]

On Mon, 14 Oct 2024 11:10:33 +0100 Edward Cree wrote:
> On 11/10/2024 19:35, Daniel Zahka wrote:
> > A list of active
> > ntuple filters and their associated rss contexts can be compiled by
> > querying a device's ethtool_ops.get_rxnfc. This patch checks to see if
> > any ntuple filters are referencing an rss context during context
> > deletion, and prevents the deletion if the requested context is still
> > in use.  
> 
> Imho it would make more sense to add core tracking of ntuple
>  filters, along with a refcount on the rss context.  That way
>  context deletion just has to check the count is zero.

True... This is my least favorite kind of situation, where the posted
code is clearly much better than the status quo, but even better
solution is possible. So question becomes do we push for the optimal
solution and potentially get neither or do we apply what's posted and
hope the better one will still be delivered later..

Re: [PATCH net-next 1/2] ethtool: rss: prevent rss ctx deletion when in use

[Daniel Zahka]

On 10/14/24 6:10 AM, Edward Cree wrote:
> Imho it would make more sense to add core tracking of ntuple
>   filters, along with a refcount on the rss context.  That way
>   context deletion just has to check the count is zero.
>
> -ed

That sounds good to me. Is that something you are planning on sending 
patches for?

Re: [PATCH net-next 1/2] ethtool: rss: prevent rss ctx deletion when in use

[Edward Cree]

On 15/10/2024 17:31, Daniel Zahka wrote:
> On 10/14/24 6:10 AM, Edward Cree wrote:
>> Imho it would make more sense to add core tracking of ntuple
>>   filters, along with a refcount on the rss context.  That way
>>   context deletion just has to check the count is zero.
>>
>> -ed
> 
> That sounds good to me. Is that something you are planning on sending patches for?

I'm afraid I don't have the bandwidth to do it any time soon.
If you aren't able to take this on, I'm okay with your original
 approach to get the issue fixed; I just wanted to ensure the
 'better' solution was considered if you do have the time for it.

Re: [PATCH net-next 1/2] ethtool: rss: prevent rss ctx deletion when in use

[Daniel Zahka]

On 10/16/24 12:23 PM, Edward Cree wrote:
> On 15/10/2024 17:31, Daniel Zahka wrote:
>> On 10/14/24 6:10 AM, Edward Cree wrote:
>>> Imho it would make more sense to add core tracking of ntuple
>>>    filters, along with a refcount on the rss context.  That way
>>>    context deletion just has to check the count is zero.
>>>
>>> -ed
>> That sounds good to me. Is that something you are planning on sending patches for?
> I'm afraid I don't have the bandwidth to do it any time soon.
> If you aren't able to take this on, I'm okay with your original
>   approach to get the issue fixed; I just wanted to ensure the
>   'better' solution was considered if you do have the time for it.
Understood. I don't have enough bandwidth to commit to implementing it 
soon either.

Re: [PATCH net-next 1/2] ethtool: rss: prevent rss ctx deletion when in use

[Paolo Abeni]

On 10/16/24 18:50, Daniel Zahka wrote:
> 
> On 10/16/24 12:23 PM, Edward Cree wrote:
>> On 15/10/2024 17:31, Daniel Zahka wrote:
>>> On 10/14/24 6:10 AM, Edward Cree wrote:
>>>> Imho it would make more sense to add core tracking of ntuple
>>>>     filters, along with a refcount on the rss context.  That way
>>>>     context deletion just has to check the count is zero.
>>>>
>>>> -ed
>>> That sounds good to me. Is that something you are planning on sending patches for?
>> I'm afraid I don't have the bandwidth to do it any time soon.
>> If you aren't able to take this on, I'm okay with your original
>>    approach to get the issue fixed; I just wanted to ensure the
>>    'better' solution was considered if you do have the time for it.
> Understood. I don't have enough bandwidth to commit to implementing it
> soon either.

I understand the chances to have a refcount based implementation soon 
are zero, and I could not find any obvious problem with the proposed 
solution, I'm going to apply this series as-is.

Cheers,

Paolo

Re: [PATCH net-next 0/2] ethtool: rss: track rss ctx busy from core

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 11 Oct 2024 11:35:46 -0700 you wrote:
> This series prevents deletion of rss contexts that are
> in use by ntuple filters from ethtool core.
> 
> Daniel Zahka (2):
>   ethtool: rss: prevent rss ctx deletion when in use
>   selftests: drv-net: rss_ctx: add rss ctx busy testcase
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] ethtool: rss: prevent rss ctx deletion when in use
    https://git.kernel.org/netdev/net-next/c/42dc431f5d0e
  - [net-next,2/2] selftests: drv-net: rss_ctx: add rss ctx busy testcase
    https://git.kernel.org/netdev/net-next/c/1ec43493c94f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html