On Thu, Sep 24, 2026 at 11:57:59AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> 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] Asymmetric teardown: `cleanup_rt_policy_ipv6()` deletes
> localsid routes it never created.
Ack, I will not remove the End localsid routes.
Thanks
Hangbin
>
> --
>
> selftest: seg6: add explicit cleanup for SRv6 End PSP test
>
> This adds cleanup_rt_policy_ipv6() and cleanup_end_flv_psp(), called at the
> end of host_srv6_end_flv_psp_tests(), to tear down the policy and localsid
> state installed for the PSP test. It also moves the host neighbour proxy
> entries from __setup_rt_policy() into the common setup().
>
> > diff --git a/tools/testing/selftests/net/srv6_end_flavors_test.sh
> > b/tools/testing/selftests/net/srv6_end_flavors_test.sh
> > index 56a1c5f865a77..52731983b4cc9 100755
> > --- a/tools/testing/selftests/net/srv6_end_flavors_test.sh
> > +++ b/tools/testing/selftests/net/srv6_end_flavors_test.sh
>
> [ ... ]
>
> > @@ -576,6 +572,56 @@ setup_rt_policy_ipv6()
> > __setup_rt_policy "$1" "$2" "$3"
> > }
> >
> > +cleanup_rt_policy_ipv6()
> > +{
> [ ... ]
> > + for n in ${policy_rts}; do
> > + node="$(__get_srv6_rtcfg_id "${n}")"
> > + op_type="$(__get_srv6_rtcfg_op "${n}")"
> > + rt_nsname="$(get_rtname "${node}")"
> > +
> > + case "${op_type}" in
> > + "noflv")
> > + function="${END_FUNC}"
> > + ;;
>
> [Severity: Medium]
> Does the "noflv" case here delete a SID that the policy helper never
> created?
>
> On the add side, __setup_rt_policy() only installs the SID if it is not
> already present:
>
> fullsid="${LOCATOR_SERVICE}:${node}::${function}"
>
> # add SRv6 Endpoint behavior to the selected router
> if ! ip -netns "${rt_nsname}" -6 route get "${fullsid}" \
> &>/dev/null; then
> ip -netns "${rt_nsname}" -6 route \
> add "${fullsid}" \
> ...
>
> For "noflv" that guard is always taken, because setup_rt_local_sids()
> already installed fcff:<rt>::${END_FUNC} for every router during the
> common setup():
>
> # Local End behavior (note that "dev" is a dummy interface chosen for
> # the sake of simplicity).
> ip -netns "${nsname}" -6 route \
> add "${LOCATOR_SERVICE}:${rt}::${END_FUNC}" \
> table "${LOCALSID_TABLE_ID}" \
> encap seg6local action End dev "${DUMMY_DEVNAME}"
>
> > +
> > + fullsid="${LOCATOR_SERVICE}:${node}::${function}"
> > +
> > + # Remove SRv6 Endpoint behavior for the selected router
> > + if ip -netns "${rt_nsname}" -6 route get "${fullsid}"
> > &>/dev/null; then
> > + ip -netns "${rt_nsname}" -6 route \
> > + del "${fullsid}" \
> > + table "${LOCALSID_TABLE_ID}" \
> > + dev "${DUMMY_DEVNAME}"
> > + fi
> > + done
>
> Since the delete is gated on the route existing rather than on whether
> this helper created it, the condition succeeds precisely for the
> setup()-owned End SID, and it is removed.
>
> [ ... ]
>
> > @@ -669,6 +723,12 @@ setup_end_flv_psp()
> > setup_rt_policy_ipv6 1 2 "1:psp"
> > }
> >
> > +cleanup_end_flv_psp()
> > +{
> > + cleanup_rt_policy_ipv6 2 1 "3:noflv 4:psp 2:psp"
> > + cleanup_rt_policy_ipv6 1 2 "1:psp"
> > +}
> > +
>
> With the "3:noflv" entry above, every normal run ends with
> fcff:3::${END_FUNC} deleted from rt-3's localsid table, so the
> environment no longer matches what setup() built. After the deletion a
> lookup of fcff:3::000e in rt-3 falls through the
>
> to fcff::/16 lookup localsid
>
> rule down to the "unreachable default" route that setup_rt_local_sids()
> installed. Should the teardown mirror the guarded add and only remove
> the flavor SIDs plus the encap route it actually created?
>
> This matters for exactly the case the commit message mentions, "when
> later tests are added to the same script": a later test that depends on
> the default End SIDs would fail or take a different path unless it
> happens to call __setup_rt_policy() with a noflv entry, which silently
> re-creates the SID. In this series the End.X PSP test added later does
> re-add fcff:3::000e via setup_endx_flv_psp(), so the asymmetry is
> currently masked and the behaviour becomes order dependent. The
> cleanup_rt_policy_ipv6() and cleanup_end_flv_psp() helpers are unchanged
> at the end of the series, still passing "3:noflv".
>
> The commit message describes the change as tearing down flavor-specific
> state the PSP test installed:
>
> "The PSP test installs policy and route state, but relies on global
> cleanup to tear it down. That can leave flavor-specific state around
> when later tests are added to the same script."
>
> Could it also mention that shared state created by setup() is removed?
>
> > check_rt_connectivity()
> > {
> > local rtsrc="$1"
> > @@ -771,6 +831,8 @@ host_srv6_end_flv_psp_tests()
> >
> > check_and_log_hs_connectivity 1 2
> > check_and_log_hs_connectivity 2 1
> > +
> > + cleanup_end_flv_psp
> > }
>
> One more question on the call site: host_srv6_end_flv_psp_tests() runs
> after "set +e", so a failing or over-reaching "ip route del" in
> cleanup_end_flv_psp() produces no test failure. Would it be worth
> checking the return values here so teardown errors are visible?
>
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-srv6_flavors_endx-v1-0-4d01c05c47b7%40kylinos.cn
