Re: [PATCH net-next 6/6] netlink: specs: rt-route: describe seg6-local actions, counters and flavors

[netdev-bot+sashiko]

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] Incomplete/misleading description of the SEG6_LOCAL_FLAVORS
  operation field: the newly added `seg6-local-flv` attribute set…

--

netlink: specs: rt-route: describe seg6-local actions, counters and flavors

This adds a seg6-local-actions enum definition, annotates the seg6-local
action attribute with it, and replaces the binary counters/flavors
attributes with nests described by the new seg6-local-cnt and
seg6-local-flv attribute sets.

> diff --git a/Documentation/netlink/specs/rt-route.yaml b/Documentation/netlink/specs/rt-route.yaml
> index 5e9537f7dfad4..3929a877aa279 100644
> --- a/Documentation/netlink/specs/rt-route.yaml
> +++ b/Documentation/netlink/specs/rt-route.yaml
> @@ -116,6 +116,29 @@ definitions:
>        - rpl
>        - ioam6
>        - xfrm
> +  -
> +    name: seg6-local-actions
> +    type: enum
> +    name-prefix: seg6-local-action-
> +    enum-name:
> +    entries:
> +      - unspec
> +      - end
> +      - end-x

[ ... ]

> +      - end-bpf
> +      - end-dt46
>  
>  sub-messages:
>    -

[ ... ]

> @@ -624,6 +650,35 @@ attribute-sets:
>        -
>          name: name
>          type: string

[ ... ]

> +  -
> +    name: seg6-local-flv
> +    name-prefix: seg6-local-flv-
> +    attributes:
> +      -
> +        name: operation
> +        type: u32

[Severity: Medium]
Should the operation attribute here also carry an enum annotation, with
enum-as-flags: true?

SEG6_LOCAL_FLV_OPERATION does not carry an ordinal, it carries a bitmask
built from the flavor-operation enum in include/uapi/linux/seg6_local.h:

enum {
	SEG6_LOCAL_FLV_OP_UNSPEC,
	SEG6_LOCAL_FLV_OP_PSP,
	SEG6_LOCAL_FLV_OP_USP,
	SEG6_LOCAL_FLV_OP_USD,
	SEG6_LOCAL_FLV_OP_NEXT_CSID,
	__SEG6_LOCAL_FLV_OP_MAX
};

net/ipv6/seg6_local.c turns those into bit positions:

#define SEG6_F_LOCAL_FLV_OP(flvname)	BIT(SEG6_LOCAL_FLV_OP_##flvname)

and seg6_local_init() confirms the u32 attribute holds one bit per
operation:

	BUILD_BUG_ON(SEG6_LOCAL_FLV_OP_MAX + 1 > BITS_PER_TYPE(__u32));

parse_nla_flavors() reads the attribute and validates it against the
per-action supported-ops mask, and put_nla_flavors() writes finfo->flv_ops
back, so both directions are masks.

As written, a user-space client generated from this spec sees an opaque
integer, and setting it to 4 for next-csid means BIT(2), i.e. USP, or
gets rejected with EINVAL depending on the action.

The sibling rt-* specs describe such fields with a flags definition plus
enum-as-flags, for example ifa-flags in
Documentation/netlink/specs/rt-addr.yaml and ifi-flags in
Documentation/netlink/specs/rt-link.yaml.

Since the same patch does add enum: seg6-local-actions for the action
attribute, the two enumerated fields of the same uAPI header end up
described inconsistently: action decodes symbolically while operation
stays a plain integer.  Would it make sense to add the matching
type: flags definition (unspec/psp/usp/usd/next-csid) here as well, given
that the generated enum names are themselves user visible and changing
them later is awkward?

> +      -
> +        name: lcblock-bits
> +        type: u8
> +      -
> +        name: lcnode-fn-bits
> +        type: u8

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917-ynl_rt_encap-v1-0-fbbe6e680571%40kylinos.cn