Re: [PATCH iproute2-next 2/3] tc: add the psp action

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 11 Nov 2026 16:15:11 -0800
Todd Freed <toddfreed@meta.com> wrote:

> The psp action applies PSP (PSP Security Protocol) encap and encryption
> to packets, separately from psp socket offload:
> 
>   psp encap assoc ASSOC_ID   attach a PSP Tx association, created with
>                              assoc-add over the PSP netlink family; the
>                              association's device encrypts the packet
>                              on transmit
>   psp decap                  remove the PSP Rx state of a packet the
>                              PSP device decrypted
> 
> decap takes no association. To take only those packets which belong to a
> single rx-spi-alloc call, a rule applies the corresponding flower psp_spi,
> psp_ver and psp_dev matches.
> 
> In JSON, the verb is psp_action. The action is documented in tc-psp(8).
> 
> Signed-off-by: Todd Freed <toddfreed@meta.com>
> ---

I saw several things that need fixing here, but AI review saw more:

New code must use strcmp(), not matches().  Six sites: "psp", "encap",
"decap", "assoc", "help", and the "index" test.  matches() does prefix
matching, so "e", "d", "a", "h", "i" all become valid keywords and are
de-facto ABI once released -- any keyword added later that shares a
prefix either changes the meaning of an existing one or can't be added.
Your own 3/3 uses strcmp(), so this is just making 2/3 match.

"psp decap assoc 3" is accepted silently and sends both
TCA_PSP_ACT_DECAP and TCA_PSP_ASSOC_ID.  Your changelog, the man page,
and your uapi comment ("u32, encap only") all say this is meaningless.
You already reject encap without assoc; please reject this too.  The
check has to go outside the "if (!p.index)" guard, since the attribute
is added unconditionally.  A repeated "assoc" is also silently
last-one-wins -- duparg() is the usual way to flag that.

print_psp() emits "psp_action" as a JSON string for encap/decap and as
a number in the default case.  Which type a script sees depends on the
kernel it's talking to.  Keep the key one type.  The default case also
fires when TCA_PSP_ACTION is absent entirely, printing "action 0".

explain() and the man page both omit "jump" from the CONTROL list,
though parse_action_control_dflt() accepts it.