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/[email protected]?part=4
---
--- Patch [4]: [PATCH net-next 4/6] net: sched: act_frer: add FRER tc action ---
commit 51190c05963e5f23d401f666c097c54aacbdc2ad
Author: Xiaoliang Yang <[email protected]>
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.
> }
