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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] net/sched: act_psp sets GSO transport_header without pulling L4
  header
- [Low] net/sched: act_psp: missing get_fill_size() callback in
  act_psp_ops
--

commit 53d19883448f901e4c3022f6ff521b64c7a129a7
Author: Todd Freed <toddfreed@meta.com>

net/sched: add act_psp for psp transport mode

This commit introduces the act_psp traffic control action to attach PSP Tx
associations via METADATA_PSP dsts on egress and strip PSP Rx extensions on
decap.

> 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
> @@ -0,0 +1,439 @@

[ ... ]

> +static bool tcf_psp_set_transport_header(struct sk_buff *skb)
> +{
> +	unsigned int off = skb_network_offset(skb);
> +	const struct iphdr *iph;
> +	unsigned int hlen, len;
> +
> +	switch (skb->protocol) {
> +	case htons(ETH_P_IP):
> +		if (!pskb_may_pull(skb, off + sizeof(*iph)))
> +			return false;
> +		iph = ip_hdr(skb);
> +		if (iph->version != 4 || iph->ihl < 5 || ip_is_fragment(iph))
> +			return false;
> +		hlen = iph->ihl * 4;
> +		len = ntohs(iph->tot_len);
> +		if (!pskb_may_pull(skb, off + hlen))
> +			return false;
> +		break;
> +	case htons(ETH_P_IPV6):
> +		if (!pskb_may_pull(skb, off + sizeof(struct ipv6hdr)))
> +			return false;
> +		if (ipv6_hdr(skb)->version != 6 ||
> +		    ipv6_ext_hdr(ipv6_hdr(skb)->nexthdr))
> +			return false;
> +		hlen = sizeof(struct ipv6hdr);
> +		len = hlen + ntohs(ipv6_hdr(skb)->payload_len);
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	/* The device takes the length of the payload from the packet: it must
> +	 * hold the IP length, and L2 padding, e.g. of a minimum size frame,
> +	 * must not be encrypted as payload. A GSO skb's header doesn't always
> +	 * carry its length (BIG TCP).
> +	 */
> +	if (!skb_is_gso(skb)) {
> +		if (len < hlen || skb->len - off < len)
> +			return false;
> +		if (pskb_trim_rcsum(skb, off + len))
> +			return false;
> +	}
> +
> +	skb_set_transport_header(skb, off + hlen);
> +	return true;
> +}

[Severity: High]
Can tcf_psp_set_transport_header() set skb->transport_header on a GSO packet
without pulling the L4 header into linear data?

tcf_psp_set_transport_header() only pulls off + hlen bytes before updating
skb->transport_header to off + hlen.  If a GSO packet enters egress with an
unset transport_header (for example, a decapsulated GSO packet bridged after
gro_cells_receive() calls skb_unset_transport_header()) or with a smaller
initial transport_header offset from virtio_net_hdr_to_skb(),
qdisc_pkt_len_segs_init() does not pull the L4 header at off + hlen.

Immediately after sch_handle_egress() in __dev_queue_xmit(), psp_xmit_md() ->
skb_gso_validate_network_len() -> skb_gso_network_seglen() ->
skb_gso_transport_seglen() calls tcp_hdrlen(skb), reading tcp_hdr(skb)->doff
past skb->tail, and mlx5e_psp_handle_tx_skb() writes to
inner_tcp_hdr(skb)->check out of bounds.  In addition, unconditionally
overwriting skb->transport_header to off + hlen defeats the
skb_network_header_len(skb) check in psp_dev_encap_ok().

[ ... ]

> +static int tcf_psp_dump(struct sk_buff *skb, struct tc_action *a,
> +			int bind, int ref)
> +{
> +	unsigned char *b = skb_tail_pointer(skb);
> +	struct tcf_psp *p = to_psp(a);
> +	struct tcf_psp_params *params;
> +	struct tc_psp opt = {
> +		.index    = p->tcf_index,
> +		.refcnt   = refcount_read(&p->tcf_refcnt) - ref,
> +		.bindcnt  = atomic_read(&p->tcf_bindcnt) - bind,
> +	};
> +	struct tcf_t t;
> +
> +	rcu_read_lock();
> +	params = rcu_dereference(p->params);
> +	opt.action = params->action;
> +
> +	if (nla_put(skb, TCA_PSP_PARMS, sizeof(opt), &opt) ||
> +	    nla_put_u32(skb, TCA_PSP_ACTION, params->psp_action))
> +		goto nla_put_failure;
> +
> +	if (params->md &&
> +	    nla_put_u32(skb, TCA_PSP_ASSOC_ID, params->md->u.psp_info.pas->id))
> +		goto nla_put_failure;
> +
> +	tcf_tm_dump(&t, &p->tcf_tm);
> +	if (nla_put_64bit(skb, TCA_PSP_TM, sizeof(t), &t, TCA_PSP_PAD))
> +		goto nla_put_failure;
> +	rcu_read_unlock();
> +
> +	return skb->len;
> +
> +nla_put_failure:
> +	rcu_read_unlock();
> +	nlmsg_trim(skb, b);
> +	return -1;
> +}
> +
> +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),
> +};

[Severity: Low]
Should act_psp_ops implement .get_fill_size?

Without .get_fill_size, tcf_action_fill_size() only budgets
tcf_action_shared_attrs_size() and does not account for TCA_PSP_PARMS,
TCA_PSP_ACTION, and TCA_PSP_ASSOC_ID (up to 40 bytes) emitted by
tcf_psp_dump(), which can under-size the notification skb allocated in
tcf_add_notify_msg(), tcf_del_notify_msg(), and tcf_get_notify() when shared
attributes such as rate estimators and 64-bit stats are present.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111005533.6436-1-toddfreed@meta.com?part=9