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.
--
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
