Re: [PATCH net-next 07/10] selftests: forwarding: tc_flower: test the enc_key_id mask

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When tcflags=skip_sw, match_enc_key_id_mask_test()
  (tools/testing/selftests/net/forwarding/tc_flower.sh) never tests…

--

selftests: forwarding: tc_flower: test the enc_key_id mask

This adds match_enc_key_id_mask_test() to tc_flower.sh. The test puts a
flower filter with enc_key_id 256/0xf00 on a gretap external device. It
checks three things: the mask shows up in the dump, a key inside the mask
matches, and a key outside the mask only hits a second exact-match filter.
Under skip_sw, if adding the masked filter fails, the test only logs it.

> diff --git a/tools/testing/selftests/net/forwarding/tc_flower.sh b/tools/testing/selftests/net/forwarding/tc_flower.sh
> index b58909a931129..9d9f109ab8954 100755
> --- a/tools/testing/selftests/net/forwarding/tc_flower.sh
> +++ b/tools/testing/selftests/net/forwarding/tc_flower.sh

[ ... ]

> +match_enc_key_id_mask_test()
> +{

[ ... ]

> +	ip link add gt-ex type gretap external # To collect tunnel info
> +	ip link set dev gt-ex up
> +	ip link set dev gt-ex master v$h2
> +	tc qdisc add dev gt-ex clsact
> +
> +	# A device may refuse a partial mask but software must take it
> +	tc filter add dev gt-ex ingress protocol ip pref 1 handle 101 flower \
> +		$tcflags enc_key_id 256/0xf00 action drop
> +	if [[ $? -ne 0 && $tcflags == "skip_sw" ]]; then
> +		log_info "Device refuses the enc_key_id mask"

[Severity: Medium]
Can the skip_sw pass ever reach enc_key_id_mask_check(), or does it always
end up in this branch?

gt-ex is a gretap external device, and ip_gre has no ndo_setup_tc. When
clsact is added on it, tcf_block_offload_bind() calls
tcf_block_offload_cmd(). That function sets up the indirect block and then
always returns an error:

net/sched/cls_api.c:tcf_block_offload_cmd() {
    ...
	flow_indr_dev_setup_offload(dev, sch, TC_SETUP_BLOCK, block, &bo,
				    tc_block_indr_cleanup);
	tcf_block_setup(block, &bo);

	return -EOPNOTSUPP;
}

As a result, tcf_block_offload_bind() does block->nooffloaddevcnt++ for
the gt-ex block.

After that, fl_hw_replace_filter() calls tc_setup_cb_add() with
err_stop set to skip_sw. tc_setup_cb_add() then rejects the filter before
any driver callback is called:

net/sched/cls_api.c:tc_setup_cb_add() {
    ...
	if (block->nooffloaddevcnt && err_stop) {
		ok_count = -EOPNOTSUPP;
		goto err_unlock;
	}
    ...
}

So every skip_sw flower filter on gt-ex would fail, whatever the mask is.
An exact enc_key_id 256 would fail the same way.

On any setup where tc_offload_check passes, the skip_sw pass would print
"Device refuses the enc_key_id mask". It would then skip filter 102 and
enc_key_id_mask_check(), and report no result.

The comment above, the log_info() text and the commit message all say
the mask is the reason:

  When tc does not know the mask or a device refuses it under skip_sw, the
  test only logs this.

Is that accurate, given that no skip_sw filter can be installed on gt-ex
at all?

This also means the skip_sw half never tests the hardware mask
programming that this series adds to cxgb4, mlx5 and nfp. A regression
there would look like the same info line.

Even without the block-level check, nfp_flower_calculate_key_layers()
looks like it would reject this filter for another reason: it matches only
enc_key_id, with no ENC_CONTROL or enc_dst_ip match. Again, that has
nothing to do with the mask.

Should the skip_sw case be dropped, or moved to a block that can actually
be offloaded?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109131039.959438-1-acassen%40corp.free.fr