Re: [PATCH net-next 4/5] selftest: seg6: add explicit cleanup for SRv6 End PSP test

[Hangbin Liu <hangbin.liu@linux.dev>]

On Thu, Sep 24, 2026 at 11:57:59AM +0000, netdev-bot+sashiko@kernel.org 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