Re: [PATCH net-next v4 3/6] netlink: specs: rt-route: add lwtunnel encap sub-message support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] rt-route is the first C-generated family whose sub-message
  selector is an integer enum (`encap-type`, u16) rather than a string;…
- [Low] Generated C parsing of route messages fails hard when
  RTA_ENCAP_TYPE carries a value outside the spec's `encap-type` enum…

--

netlink: specs: rt-route: add lwtunnel encap sub-message support

This commit adds an encap-type enum for the LWTUNNEL_ENCAP_* values. It
turns the rt-route encap attribute into a sub-message keyed on encap-type
and adds attribute sets for each lwtunnel type. It also updates
Makefile.deps to pull in the matching uapi headers.

> diff --git a/Documentation/netlink/specs/rt-route.yaml b/Documentation/netlink/specs/rt-route.yaml
> index 253037ea5176a..dc842a786794a 100644
> --- a/Documentation/netlink/specs/rt-route.yaml
> +++ b/Documentation/netlink/specs/rt-route.yaml
> @@ -99,6 +99,58 @@ definitions:
>          name: ra-withdrawn
>          doc: A Router Advertisement withdrew the route with a zero
>            lifetime.
> +  -
> +    name: encap-type
> +    type: enum
> +    name-prefix: lwtunnel-encap-
> +    enum-name:
> +    entries:
> +      - none
> +      - mpls
> +      - ip
> +      - ila
> +      - ip6
> +      - seg6
> +      - bpf
> +      - seg6-local
> +      - rpl
> +      - ioam6
> +      - xfrm

[Severity: Low]
What happens in generated C clients when the kernel reports an encap type
that is not in this list?

For an enum selector, TypeSubMessage._attr_get() in ynl_gen_c.py (added
earlier in this series) generates:

    encap_type_str = rt_route_encap_type_str(dst->encap_type);
    if (!encap_type_str)
        return ynl_submsg_failed(yarg, "encap", "enum-lookup-failed");

The generated _str() helper returns NULL for any value past the end of its
string map. lwtunnel_fill_encap() emits RTA_ENCAP_TYPE and then RTA_ENCAP
for any type up to LWTUNNEL_ENCAP_MAX.

Suppose a future kernel adds a new LWTUNNEL_ENCAP_* value. Would a single
route using that type make a C client built from this spec fail the whole
getroute dump, or fail to parse the notification?

String selectors behave differently. parse_rsp_submsg() ends its strcmp()
chain with return 0, so an unknown format is skipped. The generated parsers
also ignore unknown attribute types.

The earlier commit message in the series says this case behaves "the same
with string lookup fails". Is that accurate? Unknown string selector values
are skipped, not reported as errors.

[ ... ]

> @@ -174,9 +226,12 @@ attribute-sets:
>        -
>          name: encap-type
>          type: u16
> +        enum: encap-type
>        -
>          name: encap
> -        type: binary  # tunnel specific nest
> +        type: sub-message
> +        sub-message: encap-data
> +        selector: encap-type

[Severity: Low]
Can the C library's extack walker resolve error offsets inside RTA_ENCAP
when the selector is a u16?

rt-route looks like the first C-generated family whose sub-message selector
is an integer enum instead of a string. nftables also uses an enum
selector, but it is in GENS_UNSUP.

Selector.__init__ in ynl_gen_c.py sets attr.is_selector = True on
encap-type. However, only TypeString._attr_typol() turns that into the
policy flag:

    def _attr_typol(self):
        typol = '.type = YNL_PT_NUL_STR, '
        if self.is_selector:
            typol += '.is_selector = 1, '
        return typol

TypeScalar._attr_typol() only emits .type = YNL_PT_U16, so ynl_err_walk()
never records a selector for RTA_ENCAP.

Both newroute and delroute requests list encap-type and encap. If the
kernel returns NLMSGERR_ATTR_OFFS pointing at or inside RTA_ENCAP,
ynl_err_walk_report_one() stops here:

    if (policy->table[type].is_submsg) {
        if (!selector) {
            if (*n < str_sz)
                *n += snprintf(str, str_sz, "(!selector)");
            return 1;
        }

The user would see something like ".encap(!selector)" instead of
".encap(mpls).dst".

Setting .is_selector for scalars would not be enough by itself, because
ynl_err_walk_sel_policy() reads the selector as a string:

    sel = ynl_attr_get_str(selector);
    for (i = 0; i <= policy->max_attr; i++) {
        if (!strcmp(sel, policy->table[i].name))

That cannot match a 2-byte u16 payload.

The series patch "tools: ynl: convert enum selector to string for
sub-message parsing" updated the parse path but not this error-walk path.
Should the error walker also convert enum selectors to strings?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-ynl_rt_encap-v4-0-4ed2ccf7f949%40kylinos.cn