Re: [PATCH net-next 4/6] net: sched: act_frer: add FRER tc action

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260717095549.10565-3-xiaoliang.yang_1@nxp.com?part=4
---
--- Patch [4]: [PATCH net-next 4/6] net: sched: act_frer: add FRER tc action ---
commit 51190c05963e5f23d401f666c097c54aacbdc2ad
Author: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
Subject: net: sched: act_frer: add FRER tc action

This commit introduces the IEEE 802.1CB FRER tc action, implementing the
Sequence Generation Function and the Sequence Recovery Function. It provides
support for both cross-port shared sequence recovery and per-port individual
recovery.

> diff --git a/net/sched/act_frer.c b/net/sched/act_frer.c
> new file mode 100644
> index 0000000000000..7b6db643788df
> --- /dev/null
> +++ b/net/sched/act_frer.c

[ ... ]

> @@ -59,21 +59,21 @@ static enum hrtimer_restart frer_rcvy_hrtimer_fn(struct hrtimer *timer)
>  {
>  	struct frer_rcvy *rcvy =
>  		container_of(timer, struct frer_rcvy, hrtimer);
>  	struct frer_rcvy_work *rw;
>  
>  	/* Allocate in GFP_ATOMIC context; if it fails the state is not
>  	 * reset this cycle - the next frame will attempt again.
>  	 */
>  	rw = kmalloc_obj(*rw);

[Severity: High]
Can this allocation sleep in atomic context? The timer callback
frer_rcvy_hrtimer_fn executes in softirq context, but kmalloc_obj(*rw)
defaults to a GFP_KERNEL allocation which can sleep.

>  	if (rw) {
>  		INIT_WORK(&rw->work, frer_rcvy_reset_work_fn);
>  		rw->rcvy = rcvy;
>  		schedule_work(&rw->work);
>  	}

[Severity: High]
Is there a use-after-free risk for the tc_action here? The timer dynamically
allocates and queues a work item, but the cleanup routine only cancels the
timer. If the action is freed, the pending work could execute and access
freed memory in frer_rcvy_reset_work_fn.

>  	return HRTIMER_NORESTART;
>  }

[ ... ]

> @@ -109,51 +109,51 @@ static int frer_rtag_push(struct sk_buff *skb, u16 seq_num)
>  {

[ ... ]

>  	skb->protocol = htons(ETH_P_RTAG);
>  	skb_set_network_header(skb, head_len);
>  	if (data_offset > 0)
>  		skb_pull(skb, data_offset);

[Severity: High]
Does this leave skb->data pointing inside the R-TAG instead of the payload?
The code shifts the MAC header backwards by 6 bytes but only pulls
data_offset, leaving the pointer 6 bytes short.

>  
>  	/* Write R-TAG after the Ethernet / VLAN header */
>  	rtag = (struct r_tag *)(new_mac_header + head_len);
>  	rtag->reserved    = 0;
>  	rtag->sequence_nr = htons(seq_num);
>  	rtag->encap_proto = saved_proto;
>  
>  	return 0;
>  }

[ ... ]

> @@ -162,51 +162,51 @@ static void frer_rtag_pop(struct sk_buff *skb)
>  {

[ ... ]

>  	if (*proto_ptr != htons(ETH_P_RTAG))
>  		return;

[Severity: High]
Can this early return path leave the pointer at the MAC header? It appears
to exit without restoring skb->data using skb_pull(skb, data_offset).

>  
>  	rtag = (struct r_tag *)((unsigned char *)eh + head_len);
>  	*proto_ptr = rtag->encap_proto;
>  
>  	skb->protocol = rtag->encap_proto;
>  
>  	skb_postpull_rcsum(skb, rtag, sizeof(struct r_tag));
>  	skb_pull(skb, sizeof(*rtag));
>  	skb_reset_mac_header(skb);
>  
>  	new_mac_header = skb_mac_header(skb);
>  	memmove(new_mac_header, (unsigned char *)eh, head_len);
>  
>  	skb_set_network_header(skb, head_len);
>  	if (data_offset > 0)
>  		skb_pull(skb, data_offset);

[Severity: High]
Does this over-pull and skip 6 bytes of the payload? The code pushes
data_offset (which included the 6-byte R-TAG), pulls 6 bytes, shifts the
MAC header, and then pulls data_offset again.

>  }

[ ... ]

> @@ -273,63 +273,63 @@ static bool frer_vector_alg(struct frer_rcvy *rcvy, int seq, bool individual)
>  {

[ ... ]

>  	delta = (seq - (int)rcvy->rcvy_seq_num) &
>  		(int)(rcvy->seq_space - 1);
>  	/* Map delta > seq_space/2 to negative (signed wrap) */
>  	if ((u32)delta & (u32)(rcvy->seq_space / 2))
>  		delta -= (int)rcvy->seq_space;
>  
>  	if (delta >= (int)rcvy->history_len ||
>  	    delta <= -(int)rcvy->history_len) {
>  		/* Packet is out-of-range (rogue). */

[Severity: High]
Does this check permanently drop valid packets if history_len is configured
to 1? A valid next sequential packet with delta equal to 1 would trigger
this rogue packet check.

>  		rcvy->stats_rogue_pkts++;
>  		if (individual)
>  			restart_timer = true;
>  		pass = false;
>  		goto out;
>  	}

[ ... ]

> @@ -345,41 +345,41 @@ static bool frer_match_alg(struct frer_rcvy *rcvy, int seq, bool individual)
>  {

[ ... ]

>  	if ((u32)seq == rcvy->rcvy_seq_num) {
>  		/* Duplicate */
>  		if (individual)
>  			frer_rcvy_timer_restart(rcvy);
>  		return false;
>  	}
>  
>  	/* New sequence number: accept and update */
>  	if ((u32)seq != ((rcvy->rcvy_seq_num + 1) % rcvy->seq_space))
>  		rcvy->stats_out_of_order_pkts++;

[Severity: Low]
Does this modulo operation cause a build failure on 32-bit architectures?
The modulo operator is used with a 64-bit divisor (rcvy->seq_space), which
emits a call to __umoddi3.

>  	rcvy->rcvy_seq_num = (u32)seq;
>  	frer_rcvy_timer_restart(rcvy);
>  	return true;
>  }

[ ... ]

> @@ -407,130 +407,130 @@ static int tcf_frer_init(struct net *net, struct nlattr *nla,
>  			 struct nlattr *est, struct tc_action **a,
>  			 struct tcf_proto *tp, u32 flags,
>  			 struct netlink_ext_ack *extack)
>  {

[ ... ]

>  	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
>  	if (err < 0)
>  		goto release_idr;
>  
>  	f = to_frer(*a);
>  
>  	spin_lock_bh(&f->tcf_lock);
>  	goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
>  	f->func     = func;
>  	f->tag_type = tag_type;
>  	f->tag_pop  = !!tb[TCA_FRER_RCVY_TAG_POP];

[Severity: High]
Does this early mutation cause a resource leak and state corruption? The
code mutates live action fields and overwrites goto_chain before validating
the TCA_FRER_RCVY_ALG attribute.

If validation fails below, it jumps to release_idr without calling
tcf_chain_put_by_act(goto_ch), leaking the old chain, and leaving the action
running with a corrupted hybrid state.

>  
>  	if (func == TCA_FRER_FUNC_PUSH) {
>  		if (ret == ACT_P_CREATED) {
>  			spin_lock_init(&f->seqgen.lock);
>  			f->seqgen.seq_space = 1 << 16;
>  		}
>  		/* gen_seq_num starts at 0 on creation; preserved on replace */
>  	} else {

[Severity: High]
Does this cause a timer leak when changing modes? If a RECOVER action is
replaced with a PUSH action, f->func is changed without cancelling the old
rcvy.hrtimer. When the action is later deleted, the cleanup function will
skip timer cancellation.

>  		u8 alg = tb[TCA_FRER_RCVY_ALG] ?
>  			 nla_get_u8(tb[TCA_FRER_RCVY_ALG]) :
>  			 TCA_FRER_RCVY_VECTOR_ALG;

[ ... ]

>  		/* Cancel any running reset timer before re-initialising. */
>  		if (ret != ACT_P_CREATED && f->rcvy.reset_msec) {
>  			spin_unlock_bh(&f->tcf_lock);
>  			hrtimer_cancel(&f->rcvy.hrtimer);
>  			spin_lock_bh(&f->tcf_lock);
>  		}
>  
>  		frer_rcvy_init_state(&f->rcvy, alg, history_len,
>  				     reset_msec, take_no_seq);

[Severity: High]
Does unconditionally calling frer_rcvy_init_state() destroy active primitives?
When replacing a live RECOVER action, spin_lock_init() and hrtimer_setup()
are called while the datapath might concurrently acquire the lock or restart
the timer.

>  	}
>  
>  	spin_unlock_bh(&f->tcf_lock);
>  
>  	if (goto_ch)
>  		tcf_chain_put_by_act(goto_ch);
>  
>  	return ret;
>  
>  release_idr:
>  	tcf_idr_release(*a, bind);
>  	return err;
>  }

[ ... ]

> @@ -702,10 +702,10 @@ static void tcf_frer_cleanup(struct tc_action *a)
>  {
>  	struct tcf_frer *f = to_frer(a);
>  
>  	if (f->func == TCA_FRER_FUNC_RECOVER)
>  		hrtimer_cancel(&f->rcvy.hrtimer);

[Severity: High]
As noted above, failing to cancel the dynamic workqueue item here leaves
a pending work that will access the action memory after it is freed.

>  }