Re: [PATCH net-next v1 10/15] net/sched: flower: add psp_spi, psp_ver, and psp_dev matches

[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] The commit message misstates what a PSP flower rule matches
  once two key rotations have passed, and it overstates its claim that…

--

net/sched: flower: add psp_spi, psp_ver, and psp_dev matches

This adds a flow dissector key that is filled from the SKB_EXT_PSP Rx
state of decrypted packets. It also adds flower matches on the SPI (with
a mask), the PSP version and the PSP device ID. The goal is to let a tc
rule select the packets of a single Rx association, for example to apply
psp decap.

> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 1be6c739e3c71..95d3fa3b1568e 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c

[ ... ]

> @@ -511,6 +512,32 @@ void skb_flow_dissect_hash(const struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(skb_flow_dissect_hash);
>  
> +void skb_flow_dissect_psp(const struct sk_buff *skb,
> +			  struct flow_dissector *flow_dissector,
> +			  void *target_container)
> +{
> +#if IS_ENABLED(CONFIG_INET_PSP)
> +	struct flow_dissector_key_psp *key;
> +	struct psp_skb_ext *pse;
> +
> +	if (!dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_PSP))
> +		return;
> +
> +	pse = skb_ext_find(skb, SKB_EXT_PSP);
> +	if (!pse)
> +		return;
> +
> +	key = skb_flow_dissector_target(flow_dissector,
> +					FLOW_DISSECTOR_KEY_PSP,
> +					target_container);
> +	key->spi = pse->spi;
> +	key->dev_id = pse->dev_id;
> +	key->version = pse->version;
> +	key->present = 1;

[Severity: Medium]
Does the commit message describe the right failure mode for rules that
are still installed after two key rotations?

The commit message says:

  Together these matches tie a rule to one association, ...

and:

  this means that a rule matching packets that correspond to an rx
  association which is stale (two rotations have passed), can still match.

After two rotations the device key for the original association is gone.
Its packets then fail to decrypt and never get SKB_EXT_PSP, so the stale
association itself can no longer match.

The SPI value can be handed out again, though. In netdevsim, for example:

drivers/net/netdevsim/psp.c:nsim_key_rotate() {
    ...
	if (ns->psp.spi & PSP_SPI_KEY_PHASE)
		ns->psp.spi = 0;
	else
		ns->psp.spi = PSP_SPI_KEY_PHASE;
    ...
}

and nsim_rx_spi_alloc() then does:

	assoc->spi = cpu_to_be32(++ns->psp.spi);

So a new rx-spi-alloc, possibly for a different peer, can get 0x80000001
again. skb_flow_dissect_psp() only copies spi, dev_id and version. Given
that, wouldn't the example rule from the commit message:

  psp_spi 0x80000001 psp_ver 0 psp_dev 1 action psp decap

strip PSP state from the traffic of a different, live association? With
a mirred action instead of decap, it would forward that traffic.

The socket path avoids this because psp_pse_matches_pas() also compares
the generation:

	return pse && pas->rx.spi == pse->spi &&
	       pas->generation == pse->generation &&
	       pas->version == pse->version &&
	       pas->dev_id == pse->dev_id;

Adding generation to the key would not fully fix this either. The mlx5
Rx path always passes generation = 0 to psp_dev_rcv(). So it looks like
(spi, version, dev) cannot identify a single association beyond two
rotations on any driver.

The "Socket-less associations" text that a later patch in this series
adds to Documentation/networking/psp.rst has the same gap. It only says
userspace should replace such rules before two rotations, and it repeats
that the matches tie a decap rule to one Rx association. No later patch
in the series adds a generation match or any enforcement.

Could the commit message and the documentation say that a stale rule can
end up matching a different association that reused the SPI, rather
than the stale association itself?

[ ... ]

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