Skip to content

.pr_agent_accepted_suggestions

qodo-merge-bot edited this page Aug 12, 2026 · 2 revisions
                     PR 695 (2026-08-12)                    
[reliability] Early srv6 nexthop deref
Early srv6 nexthop deref srv6_output_process() now calls nexthop_info_srv6_output(nh) (and in traced case dereferences d->seglist[0]) before validating that the packet is IPv4/IPv6, so packets that would take the INVALID edge can instead hit an assert or undefined memory access. This changes failure mode from clean INVALID enqueue to process termination/UB when nh is NULL/wrong-type or SRv6 fields are not valid yet.

Issue description

srv6_output_process() now reads nh = l3_mbuf_data(m)->nh; and calls nexthop_info_srv6_output(nh) before checking whether the packet is IPv4/IPv6. The function generated by GR_NH_TYPE_INFO asserts on nh->type and has no NULL guard, so packets that should go to the existing INVALID edge can terminate the dataplane (assert) or trigger undefined behavior.

Additionally, traced packets unconditionally read d->seglist[0] while still before the packet-type validation.

Issue Context

The node already has an explicit INVALID path for non-IPv4/non-IPv6 packets; the SRv6 nexthop lookup should not run before that validation.

Fix Focus Areas

  • modules/srv6/datapath/srv6_output.c[59-90]
  • modules/infra/control/nexthop.h[33-38]

Suggested fix

  1. Move nh = ...; d = ...; (and the trace population) to after the IPv4/IPv6 packet-type branch has accepted the packet.
  2. Or, if you want to keep early resolution, add an explicit guard before calling nexthop_info_srv6_output(nh):
    • if nh == NULL or nh->type != GR_NH_T_SR6_OUTPUT, set edge = INVALID (or a more specific edge) and goto next.
  3. When filling trace, only read d->seglist[0] if d->n_seglist > 0 (and consider formatting seg[0] only in that case).


                     PR 684 (2026-08-06)                    
[correctness] Hash key has garbage bytes
Hash key has garbage bytes set_srv6_local_key() no longer fully initializes struct srv6_local_key, but rte_hash uses sizeof(struct srv6_local_key) bytes as the key; any unwritten bytes (e.g., struct padding) become part of the key. This can cause intermittent lookup/add/del mismatches and stale/duplicate entries for logically identical SRv6-local nexthops.

Issue description

srv6_local_hash keys are built from struct srv6_local_key, but set_srv6_local_key() does not zero/initialize the full key object. Since the hash key length is sizeof(struct srv6_local_key), any bytes not explicitly written (including any padding) are hashed/compared, leading to nondeterministic behavior.

Issue Context

This affects SRv6-local nexthop lookup, insertion, deletion, and the memcmp(old_key, key) comparison.

Fix Focus Areas

  • modules/srv6/control/localsid.c[20-34]
  • modules/srv6/control/localsid.c[40-45]

What to change

  • Ensure set_srv6_local_key() produces a fully deterministic byte representation:
    • memset(key, 0, sizeof(*key)); then assign fields, or
    • Build a packed/byte-array key and fill every byte explicitly.
  • Prefer copying individual fields from info (rather than struct assignment) if you want to avoid propagating any padding from the source struct into the key.

[correctness] Wrong old key deleted
Wrong old key deleted srv6_local_nh_import_info() computes old_key using &nh->base, but nexthop_update() overwrites nh->base before calling import_info(). If vrf_id/iface_id change during an update-by-id, rte_hash_del_key() can delete an unrelated entry (new base + old info) and fail to delete this nexthop’s real old mapping (old base), leaving stale hash entries.

Issue description

srv6_local_nh_import_info() tries to remove the previous hash entry by rebuilding old_key from &nh->base + priv->base. But at this point nh->base already contains the new base (updated by nexthop_update()), so old_key may not match the actually inserted old key (which used the prior vrf/iface).

This can:

  • leave the true old mapping behind (stale lookup path), and/or
  • delete another nexthop’s entry if it happens to match the mixed (new base + old info) key.

Issue Context

nexthop_update() assigns nh->base = *base before calling ops->import_info(nh, info).

Fix Focus Areas

  • modules/srv6/control/localsid.c[150-175]
  • modules/infra/control/nexthop.c[340-376]

What to change

Pick one:

  1. Store the last key base fields in the SRv6-local private area (e.g., persist key_vrf_id/key_iface_id alongside priv->base) and use those stored values to construct old_key.
  2. Change the infra update API so import_info receives both old and new base, or so the type layer can delete the old mapping before nh->base is overwritten.

Ensure rte_hash_del_key() is called with the actual previously-inserted key bytes.