Re: [PATCH net-next v1 14/15] selftests: tc-testing: add tests for the psp action

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Tests 2d50 ("Add psp action without encap or decap"), cab5 ("Add
  psp encap action without assoc, with index") and 0c17 ("Reject psp…
- [Low] Tests a744 ("Add flower psp_spi filter with psp decap") and 4e1f
  ("Add flower psp_spi, psp_ver and psp_dev filter with psp decap") in…

--

selftests: tc-testing: add tests for the psp action

This adds tdc tests for the psp tc action. They add, replace, delete and
flush decap actions, and check that invalid encap/decap configurations are
rejected. It also adds flower filter tests for psp_spi, psp_ver and
psp_dev, and enables CONFIG_INET_PSP and CONFIG_NET_ACT_PSP in the
tc-testing config.

> diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/psp.json b/tools/testing/selftests/tc-testing/tc-tests/actions/psp.json
> new file mode 100644
> index 0000000000000..990e8bd1ce9a0
> --- /dev/null
> +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/psp.json
> @@ -0,0 +1,459 @@

[ ... ]

> +    {
> +        "id": "2d50",
> +        "name": "Add psp action without encap or decap",

[ ... ]

> +        "cmdUnderTest": "$TC actions add action psp index 12",
> +        "expExitCode": "255",
> +        "verifyCmd": "$TC actions list action psp",
> +        "matchPattern": "action order [0-9]+: psp",
> +        "matchCount": "0",

[Severity: Low]
Does this test reach the kernel check it is named after?

The test passes if the command exits with 255 and no psp action is listed
afterwards. tc actions add also exits with 255 when tc's own parser refuses
the arguments, before any netlink request is sent. In iproute2,
tc_action_modify() returns -1 for a parse_action() failure and for an
rtnl_talk() failure alike.

If the psp parser in tc requires encap or decap, this test would pass
without ever reaching this check:

net/sched/act_psp.c:tcf_psp_init() {
    ...
	if (NL_REQ_ATTR_CHECK(extack, nla, tb, TCA_PSP_ACTION)) {
		NL_SET_ERR_MSG(extack, "Missing PSP action, encap or decap");
    ...
}

The same question applies to two other tests:

- cab5 ("Add psp encap action without assoc, with index") and the
  "Missing PSP association ID" check in the encap case.
- 0c17 ("Reject psp decap action with an assoc") and the "decap takes no
  PSP association" check in the decap case.

In each case the test would still pass if the kernel branch were removed.

The flower tests b58d and 2f96 avoid this by expecting exit code 2.
tc filter add returns 2 only on an rtnl_talk() error and returns 1 on a
parse failure. The action tests have no such check.

Would it make sense to match the extack text instead? For example, a
verifyCmd could rerun the command with stderr captured and grep for the
kernel message.

[ ... ]

> +    {
> +        "id": "a744",
> +        "name": "Add flower psp_spi filter with psp decap",

[ ... ]

> +        "dependsOn": "$TC filter add flower help 2>&1 | grep -q psp_spi",

[ ... ]

> +        "cmdUnderTest": "$TC filter add dev $DEV1 ingress protocol ip pref 1 flower psp_spi 0x1234 action psp decap",
> +        "expExitCode": "0",

[Severity: Low]
Should this dependsOn also check for the psp action?

The command uses "action psp decap", but the probe only checks that flower
knows psp_spi. The action tests earlier in this file probe for the action
like this:

    $TC actions add action psp help 2>&1 | grep -q decap

run_one_test() in tdc.py uses only the probe's return code to decide
whether to skip a test. Suppose tc has the flower psp_* keys but not the
psp action. This test would then run instead of being skipped. tc would
fall back to its noact parser, reject the action, and the test would be
reported as a failure.

The commit message says:

    The tests need a tc with the psp action and the flower psp_* keys; they
    skip without it (dependsOn).

Test 4e1f ("Add flower psp_spi, psp_ver and psp_dev filter with psp decap")
has the same issue.

Could both probes be chained with && for these two tests?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111005533.6436-1-toddfreed%40meta.com