Re: [PATCH net-next 2/5] seg6: add RFC8986 flavor support for End.X

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

Hi,
On Thu, Sep 24, 2026 at 11:57:56AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> 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] net/ipv6/seg6_local.c: the patch widens
>   SEG6_LOCAL_END_X_FLV_SUPP_OPS to (SEG6_F_LOCAL_FLV_NEXT_CSID |…

Andrea has talked about this issue and will fix it in net tree.

> - [Low] net/ipv6/seg6_local.c:759-761 — the kerneldoc-style comment on
>   end_flv8986_core() still says the helper processes the packet "on the…

OK, I will update the comment.

Thanks
Hangbin
> 
> --
> 
> seg6: add RFC8986 flavor support for End.X
> 
> This adds PSP processing to the SRv6 End.X behavior by reusing
> end_flv8986_core(), and widens SEG6_LOCAL_END_X_FLV_SUPP_OPS so End.X
> advertises PSP flavor support alongside NEXT-C-SID.
> 
> > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> > index 584e6aca38930..7462da1da3629 100644
> > --- a/net/ipv6/seg6_local.c
> > +++ b/net/ipv6/seg6_local.c
> > @@ -121,7 +121,8 @@ struct bpf_lwt_prog {
> >  
> >  #define SEG6_LOCAL_END_FLV_SUPP_OPS	(SEG6_F_LOCAL_FLV_NEXT_CSID | \
> >  					 SEG6_LOCAL_FLV8986_SUPP_OPS)
> > -#define SEG6_LOCAL_END_X_FLV_SUPP_OPS	SEG6_F_LOCAL_FLV_NEXT_CSID
> > +#define SEG6_LOCAL_END_X_FLV_SUPP_OPS	(SEG6_F_LOCAL_FLV_NEXT_CSID | \
> > +					 SEG6_LOCAL_FLV8986_SUPP_OPS)
> 
> [Severity: Medium]
> With this mask widened, does parse_nla_flavors() now accept the
> combination next-csid plus psp for an End.X instance?
> 
> parse_nla_flavors() only rejects bits outside the per-action mask:
> 
> 	rc = seg6_flv_supp_ops_by_action(action, &supp_fops);
> 	if (rc < 0 || (fops & ~supp_fops)) {
> 		NL_SET_ERR_MSG(extack, "Unsupported Flavor operation(s)");
> 		return -EOPNOTSUPP;
> 	}
> 
> 	finfo->flv_ops = fops;
> 
> and seg6_flv_supp_ops_by_action() returns SEG6_LOCAL_END_X_FLV_SUPP_OPS for
> SEG6_LOCAL_ACTION_END_X, so "ip route ... action End.X nh6 <addr> oif <dev>
> flavors next-csid,psp" would be installed, and put_nla_flavors() echoes the
> stored mask including PSP back to user space.
> 
> Is PSP actually executed in that configuration?  In the new
> input_action_end_x() below, the NEXT-C-SID test returns before
> end_flv8986_core() is reached, and the arg-zero sub-case of
> end_x_next_csid_core() goes to input_action_end_x_core():
> 
> 	if (seg6_next_csid_is_arg_zero(daddr, finfo))
> 		return input_action_end_x_core(skb, slwt);
> 
> input_action_end_x_core() only does advance_nextseg() and then
> input_action_end_x_finish(), so seg6_pop_srh() is never called and the SRH
> stays on the packet even though PSP was accepted and is reported as
> configured.  The comment above seg6_flv_supp_ops_by_action() states that
> unsupported combinations make the kernel refuse to instantiate the tunnel.
> 
> Would it be better to reject next-csid together with psp for End.X in
> parse_nla_flavors(), or to route the NEXT-C-SID arg-exhausted path into
> end_flv8986_core()?
> 
> For reference, the same shape already exists for the End action via
> SEG6_LOCAL_END_FLV_SUPP_OPS and input_action_end()/end_next_csid_core(),
> but for End.X the combination only becomes configurable with this change.
> At the end of the series the mask and input_action_end_x() are unchanged,
> and the added selftest only installs psp on its own, so the combination
> stays accepted but unimplemented.
> 
> >  
> >  struct seg6_flavors_info {
> >  	/* Flavor operations */
> > @@ -841,12 +842,19 @@ static int input_action_end_x(struct sk_buff *skb, struct seg6_local_lwt *slwt)
> >  {
> >  	const struct seg6_flavors_info *finfo = &slwt->flv_info;
> >  	__u32 fops = finfo->flv_ops;
> > +	int ret;
> > +
> > +	if (!fops)
> > +		return input_action_end_x_core(skb, slwt);
> >  
> >  	/* check for the presence of NEXT-C-SID since it applies first */
> >  	if (seg6_next_csid_enabled(fops))
> >  		return end_x_next_csid_core(skb, slwt);
> >  
> > -	return input_action_end_x_core(skb, slwt);
> > +	ret = end_flv8986_core(skb, slwt);
> > +	if (ret)
> > +		return ret;
> > +	return input_action_end_x_finish(skb, slwt);
> >  }
> 
> [Severity: Low]
> This isn't a bug, but the comment above end_flv8986_core() still reads:
> 
> 	/* process the packet on the basis of the RFC8986 flavors set for the given
> 	 * SRv6 End behavior instance.
> 	 */
> 
> With this new call site the helper also serves End.X, whose post-processing
> differs (input_action_end_x_finish() forwards to slwt->nh6/slwt->oif rather
> than doing a DA-based lookup).  Could the comment say "End* behavior", which
> is the wording already used above seg6_flv_supp_ops_by_action() and in the
> SEG6_F_LOCAL_FLAVORS comment?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-srv6_flavors_endx-v1-0-4d01c05c47b7%40kylinos.cn