Re: [PATCH net-next v1 15/15] Documentation: networking: psp: document socket-less associations

[netdev-bot+sashiko]

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] The new psp.rst text says, in the `psp encap assoc ID` bullet,
  "Encap on ingress is hardware offload only." The header comment of…
- [Low] In psp.rst, the `psp encap assoc ID` bullet says: "If the
  association is removed before transmit, or if the packet is…
- [Low] In the flower paragraph, psp.rst says the `psp_spi`, `psp_ver`
  and `psp_dev` matches together "tie a decap rule to one Rx…

--

Documentation: networking: psp: document socket-less associations

This commit reorganizes the PSP documentation around associations. It
documents the new netlink commands for socket-less associations, the psp
tc action and the flower psp_spi, psp_ver and psp_dev matches. It also
adds sections on fragments and on PSP device statistics.

> diff --git a/Documentation/networking/psp.rst b/Documentation/networking/psp.rst
> index 0f9b6b73f2447..79b101db5dfc4 100644
> --- a/Documentation/networking/psp.rst
> +++ b/Documentation/networking/psp.rst

[ ... ]

> @@ -140,6 +162,55 @@ The PSP assoc state of a socket is not reset when the connection is
>  torn down. ``connect()`` on a socket that has PSP assoc state will
>  return ``-EINVAL``.
>  
> +Socket-less associations
> +~~~~~~~~~~~~~~~~~~~~~~~~
> +

[ ... ]

> +- ``assoc-del`` deletes an association. tc actions and packets which still
> +  reference it keep it alive. The device keeps decrypting the SPI of a
> +  deleted Rx association until its device key is rotated out: remove the
> +  decap rule to stop taking its packets.

[ ... ]

> +- ``psp encap assoc ID`` attaches a Tx association. The packet must then
> +  be transmitted on the main netdev of the association's PSP device, which
> +  encrypts it (``mirred egress redirect``). If the association is removed
> +  before transmit, or if the packet is transmitted on any other device,
> +  it will be dropped, rather than sent in the clear. Encap on ingress is
> +  hardware offload only.

[Severity: Low]
Is the part that says "If the association is removed before transmit ...
it will be dropped" accurate? It conflicts with the assoc-del bullet above
and with the assoc-del doc in Documentation/netlink/specs/psp.yaml:

    Packets and tc actions which still reference it keep it alive.

In the code, psp_nl_assoc_del_doit() only calls psp_assoc_unpublish().
That removes the ID from the registry and drops the registry's reference:

net/psp/psp_assoc.c:psp_assoc_unpublish() {
    ...
	if (xa_cmpxchg(&psp_assocs, pas->id, pas, NULL, 0) == pas)
		psp_assoc_put(pas);
    ...
}

The METADATA_PSP dst created by tcf_psp_md_create() keeps its own
reference. tcf_psp_act() also keeps attaching dst_clone(&params->md->dst)
to every packet.

On transmit, psp_xmit_md() checks only the device and the MTU. It never
checks whether the association was deleted:

include/net/psp/functions.h:psp_xmit_md() {
    ...
	if (rcu_access_pointer(dev->psp_dev) != pas->psd ||
	    dev != pas->psd->main_netdev)
		return -EINVAL;
    ...
}

So after assoc-del, packets are still encrypted and sent with the deleted
association's key until the tc action is removed. A drop seems to happen
only when the packet loses its METADATA_PSP dst, or when the PSP device
does not match or has been unregistered.

Would "if the packet loses its association on the way (e.g. its dst is
replaced)" describe this more accurately?

[Severity: Low]
Is "Encap on ingress is hardware offload only" accurate for this series?
act_psp_ops in net/sched/act_psp.c does not set .offload_act_setup:

static struct tc_action_ops act_psp_ops = {
	.kind		=	"psp",
	.id		=	TCA_ID_PSP,
	.owner		=	THIS_MODULE,
	.act		=	tcf_psp_act,
	.dump		=	tcf_psp_dump,
	.init		=	tcf_psp_init,
	.cleanup	=	tcf_psp_cleanup,
	.size		=	sizeof(struct tcf_psp),
};

Because of that, tc_setup_offload_act() in net/sched/cls_api.c rejects the
action:

	} else {
		NL_SET_ERR_MSG(extack, "Action does not support offload");
		return -EOPNOTSUPP;
	}

In software, tcf_psp_act() always drops encap at ingress:

		if (unlikely(skb_at_tc_ingress(skb)))
			return tcf_psp_drop(p, skb, SKB_DROP_REASON_PSP_OUTPUT);

So ingress encap cannot work in any configuration today. The flower commit
in this series says offload will come in a follow-up series.

Should this say that ingress encap is not supported (the packet is always
dropped) and that hardware offload is future work? The header comment in
net/sched/act_psp.c makes the same claim.

[ ... ]

> +The flower classifier matches what a packet was decrypted with:
> +``psp_spi SPI[/MASK]``, ``psp_ver VERSION`` and ``psp_dev DEV_ID``, the
> +ID of the PSP device which decrypted it. These never match a clear text
> +packet, so ``psp_spi 0/0`` matches any packet decrypted with PSP.
> +Together they tie a decap rule to one Rx association.
> +
> +Once the device key an SPI was created from has been rotated twice, that
> +association is considered stale. Unlike sockets, tc does not handle key
> +rotation. userspace should replace such rules before two rotations.

[Severity: Low]
Does "Together they tie a decap rule to one Rx association" overstate what
the match guarantees? The flow key built by skb_flow_dissect_psp() carries
only these fields:

	key->spi = pse->spi;
	key->dev_id = pse->dev_id;
	key->version = pse->version;
	key->present = 1;

It has no key generation and no association identity. SPI values get
reused after key rotation. For example, nsim_key_rotate() in netdevsim
resets the SPI counter:

	if (ns->psp.spi & PSP_SPI_KEY_PHASE)
		ns->psp.spi = 0;
	else
		ns->psp.spi = PSP_SPI_KEY_PHASE;

After two rotations, the same SPI can be given to a different peer or to a
socket association under a new key. If the old decap rule is still in
place, it would strip SKB_EXT_PSP and skb->decrypted from that other
association's packets and forward them.

PSP device IDs are also allocated cyclically in a 16-bit space in
psp_dev_create():

	err = xa_alloc_cyclic(&psp_devs, &psd->id, psd, xa_limit_16b,
			      &last_id, GFP_KERNEL);

After enough unregister/register cycles, a stale psp_dev match could end
up matching a different device.

The next paragraph says userspace should replace rules before two
rotations. It does not say that a stale rule will match other
associations' traffic, and it does not mention device ID reuse.

The flower commit in this series says "the generation of the device key is
not checked". Could the doc include the same qualification here?

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