Re: [PATCH net-next v1 09/15] net/sched: add act_psp for psp transport mode

[netdev-bot+sashiko]

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

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

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] ENCAP in tcf_psp_act() relies on two marks staying together:
  the METADATA_PSP dst, which requests encryption, and skb->decrypted,…
- [Low] The act_psp.c file header ("In software, egress only: like tunnel
  mode, encap on ingress is hardware offload only"), the comment in the…
- [Low] The block comment above sk_validate_xmit_skb() in net/core/dev.c
  still says "If the skb belongs to an HW offloaded socket, apply the…

--

net/sched: add act_psp for psp transport mode

This adds a tc action, act_psp. Its encap mode attaches a PSP Tx
association to egress packets as a METADATA_PSP dst and sets
skb->decrypted, so the PSP device encrypts them on transmit. Its decap
mode removes the PSP Rx state from packets the device decrypted, and
sk_validate_xmit_skb() now drops METADATA_PSP packets whose socket has its
own validator.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2cca5f0d302f8..353c0409b3c03 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4154,13 +4154,22 @@ static struct sk_buff *sk_validate_xmit_skb(struct sk_buff *skb,

[ ... ]

>  	if (sk_validate) {
> -		skb = sk_validate(sk, dev, skb);
> +		/* act_psp doesn't give an association to the packets of a
> +		 * socket with its own offload, but the socket can gain one
> +		 * while such a packet is queued
> +		 */
> +		if (unlikely(skb_psp_md(skb))) {
> +			kfree_skb_reason(skb, SKB_DROP_REASON_PSP_OUTPUT);
> +			skb = NULL;
> +		} else {
> +			skb = sk_validate(sk, dev, skb);
> +		}

[Severity: Low]
This isn't a bug, but the block comment above sk_validate_xmit_skb()
still says:

  /* Validate an skb for device encryption offload. If the skb belongs to an HW
   * offloaded socket, apply the socket's callback. Otherwise, an skb which
   * carries a PSP association is checked again against dev: ...

With this new branch, an skb whose socket has a validator and which also
carries a METADATA_PSP dst is freed with SKB_DROP_REASON_PSP_OUTPUT. The
socket's callback never runs for it.

Should the function comment be updated to cover this case?

[ ... ]

> diff --git a/net/sched/act_psp.c b/net/sched/act_psp.c
> new file mode 100644
> index 0000000000000..4ff54203c9f51
> --- /dev/null
> +++ b/net/sched/act_psp.c

[ ... ]

> +static int tcf_psp_act(struct sk_buff *skb, const struct tc_action *a,
> +		       struct tcf_result *res)
> +{

[ ... ]

> +	switch (params->psp_action) {
> +	case TCA_PSP_ACT_ENCAP:
> +		/* Encap on ingress is hardware offload only, like tunnel
> +		 * mode: the packet would go through the stack with nothing
> +		 * but its dst, which routing, netfilter or a redirect may
> +		 * replace, to keep it from leaving in clear text
> +		 */
> +		if (unlikely(skb_at_tc_ingress(skb)))
> +			return tcf_psp_drop(p, skb, SKB_DROP_REASON_PSP_OUTPUT);

[Severity: Low]
Is there a hardware offload path for this action?

This comment, the file header ("In software, egress only: like tunnel
mode, encap on ingress is hardware offload only"), the commit message and
Documentation/networking/psp.rst all say ingress encap is available through
hardware offload.

act_psp_ops doesn't set .offload_act_setup, so tc_setup_offload_act()
rejects the action:

net/sched/cls_api.c:tc_setup_offload_act() {
	if (act->ops->offload_act_setup) {
		return act->ops->offload_act_setup(act, entry, index_inc, true,
						   extack);
	} else {
		NL_SET_ERR_MSG(extack, "Action does not support offload");
		return -EOPNOTSUPP;
	}
}

include/net/flow_offload.h has no PSP flow_action_id either. The PSP
tunnel mode that "like tunnel mode" refers to isn't in the tree yet; the
commit message says a later series adds it.

So skip_sw rules are rejected. Software rules install without error,
because tcf_psp_init() has no direction check, and then every matching
ingress packet is dropped with SKB_DROP_REASON_PSP_OUTPUT.

Could the comments, commit message and psp.rst say that ingress encap is
not supported, rather than hardware offload only?

> +		/* PSP on a tunnel isn't supported: the encap would overwrite
> +		 * the inner headers which the device's tunnel offloads use
> +		 */

[ ... ]

> +		skb_dst_drop(skb);
> +		skb_dst_set(skb, dst_clone(&params->md->dst));
> +		/* Must be encrypted: sk_validate_xmit_skb() drops it unless
> +		 * it still carries the association when it is transmitted
> +		 */
> +		skb->decrypted = 1;

[Severity: Medium]
Can the METADATA_PSP dst and skb->decrypted get separated after this
point?

The commit message relies on skb->decrypted as the backstop: "on egress,
encap sets skb->decrypted, so that sk_validate_xmit_skb() drops the packet
if it loses the METADATA_PSP dst on the way". The egress to ingress guards
in mirred and bpf also check only the decrypted bit:

net/sched/act_mirred.c:tcf_mirred_to_dev() {
	if (unlikely(want_ingress && !at_ingress && skb_is_decrypted(skb)))
		goto err_cant_do;
}

ip_copy_metadata() copies the dst but not the decrypted bit, so fragments
built by ip_do_fragment() end up with decrypted == 0:

net/ipv4/ip_output.c:ip_copy_metadata() {
	...
	skb_dst_drop(to);
	skb_dst_copy(to, from);
	...
	skb_ext_copy(to, from);
	...
}

One path where this happens, on a non-PSP netdev's clsact egress:

  act_ct (defrag, sets tc_skb_cb(skb)->mru)
  -> psp encap (the reassembled packet is not a fragment, so it is accepted)
  -> mirred egress redirect to the PSP netdev

tcf_dev_queue_xmit()
  sch_frag_xmit_hook()
    sch_fragment()
      ip_do_fragment(net, skb->sk, skb, sch_frag_xmit)

sch_frag_xmit() then puts the original dst back on each fragment:

	__skb_dst_copy(skb, data->dst);

As a result, every fragment carries the association but has
decrypted == 0.

In the direct case the PSP driver drops the fragments. Suppose a later step
on the target's egress replaces the dst, for example tunnel_key set plus a
redirect to a tunnel device. sk_validate_xmit_skb() then sees neither the
md dst nor the decrypted mark, and the packet goes out in clear text. A
redirect to ingress would also pass the skb_is_decrypted() check above.

The TCA_PSP_ACT_DECAP branch below can split the two marks as well. It
clears skb->decrypted whenever SKB_EXT_PSP is present, without checking for
an attached METADATA_PSP dst.

Would it make sense to do one or more of the following?

- also test skb_psp_md() in the mirred/bpf guards and in the cleartext
  backstop
- propagate decrypted in ip_copy_metadata() and ip6_copy_metadata()
- have decap refuse packets that carry a METADATA_PSP dst

> +		break;
> +	case TCA_PSP_ACT_DECAP:
> +		/* Only a packet the device decrypted: on transmit,
> +		 * skb->decrypted means "device, please encrypt", which decap
> +		 * must not undo.
> +		 */
> +		if (!skb_ext_find(skb, SKB_EXT_PSP))
> +			break;
> +		skb_ext_del(skb, SKB_EXT_PSP);
> +		skb->decrypted = 0;
> +		break;
> +	}

[ ... ]

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