Re: [PATCH net-next v4 2/6] tools: ynl: convert enum selector to string for sub-message parsing

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

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] An unknown enum selector value now aborts the whole message or
  dump.
- [Medium] The new enum-to-string conversion ignores byte order.
- [Low] When the enum lookup fails, the error message is misleading.
- [Low] The generator now produces working response parsing for enum-
  keyed sub-messages.

--

tools: ynl: convert enum selector to string for sub-message parsing

This patch changes the YNL C code generator for sub-messages keyed by a
non-external enum selector. The integer selector value is converted to a
string with the family's {enum}_str() helper before the sub-message parse
function is called. A missing selector or a failed enum lookup is
reported through ynl_submsg_failed().

> diff --git a/tools/net/ynl/pyynl/ynl_gen_c.py b/tools/net/ynl/pyynl/ynl_gen_c.py
> index 15c79849c6096..4aca51396a9e2 100755
> --- a/tools/net/ynl/pyynl/ynl_gen_c.py
> +++ b/tools/net/ynl/pyynl/ynl_gen_c.py
> @@ -951,13 +951,31 @@ class TypeSubMessage(TypeNest):

[ ... ]

> +        if self.selector.is_enum_val() and not self.selector.is_external():
> +            enum = self.family.consts[self.selector.get_enum_name()]
> +            pres_var = f"{var}->_present.{sel}"
> +            parse_sel = f"{sel}_str"
> +            local_vars = [f'const char *{parse_sel};']
> +
> +            get_lines = [
> +                f'if (!{pres_var})',
> +                f'return ynl_submsg_failed(yarg, "{self.name}", "{selector}");',
> +                f'{parse_sel} = {enum.render_name}_str({sel_var});',

[Severity: Medium]
Does this handle selectors declared with byte-order: big-endian?

TypeScalar._attr_get() loads the value in {sel_var} with a plain
ynl_attr_get_u32() or ynl_attr_get_u16(), which is a native load. In
TypeScalar.__init__(), byte-order only becomes a C comment through
byte_order_comment. The struct member therefore holds the value in wire
order.

On a little-endian host, a big-endian selector value of 1 arrives as
0x01000000. That fails the range check in the generated {enum}_str():

    if (value < 0 || value >= (int)YNL_ARRAY_SIZE(map))
        return NULL;

So every sub-message would fail with "enum-lookup-failed".

This combination already exists in-tree. obj-attrs in
Documentation/netlink/specs/nftables.yaml has:

    name: type
    type: u32
    enum: object-type
    byte-order: big-endian
    ...
    name: data
    type: sub-message
    sub-message: obj-data
    selector: type

nftables is listed in GENS_UNSUP in tools/net/ynl/generated/Makefile, so
the default build does not generate it.

Before this patch, though, the generator passed the u32 as the
const char *sel argument, which failed at compile time. Now it silently
emits code that cannot decode these sub-messages.

Should the conversion normalize the byte order first, or should the
generator reject such specs?

Because of obj-attrs, the note below the commit message saying rt-route
encap is "the first enum-keyed sub-message in the YNL specs" also does
not seem accurate.

> +                f'if (!{parse_sel})',
> +                f'return ynl_submsg_failed(yarg, "{self.name}", "enum-lookup-failed");']

[Severity: Medium]
Is it intended that an unknown enum value aborts the whole message or
dump?

ynl_submsg_failed() always returns YNL_PARSE_CB_ERROR.
ynl_dump_trampoline() passes that up, and __ynl_sock_read_msgs() stops
processing.

The string selector path behaves differently. parse_rsp_submsg() emits a
strcmp() chain that ends in return 0. An unknown string key, such as a
new rt-link kind, is skipped and parsing continues.

The enum path is also inconsistent within itself. A value that is in the
enum but has no formats entry falls through the strcmp() chain and
returns 0. A value outside the enum is a hard error.

Take the rt-route spec from this series. If a future kernel adds a new
LWTUNNEL_ENCAP_* type, wouldn't the generated client fail the entire
route dump as soon as one route uses that encap? Leaving the encap
undecoded would match the rest of YNL.

The commit message says:

    or a new encap_type is missing from the spec file in future kernel,
    the code will report "Sub-message key not set", the same with string
    lookup fails.

This doesn't match the string path, which skips unknown keys silently.

Could this skip the sub-message instead, leaving it unset and returning
success, when the enum lookup misses?

[Severity: Low]
Is "enum-lookup-failed" the right value for the selector name argument?
ynl_submsg_failed() always uses this format with YNL_ERROR_SUBMSG_KEY:

    "Parsing error: Sub-message key not set (msg %s, key %s)"

The user would see "Sub-message key not set (msg encap, key
enum-lookup-failed)". In this case the key was present but held an
unknown value.

The message also loses the real selector name (encap-type) and does not
report the numeric value that failed. To the caller, this looks the same
as a missing selector.

> +        else:
> +            parse_sel = sel_var

[ ... ]

> @@ -979,6 +997,12 @@ class Selector:
>      def is_external(self):
>          return self._external
>  
> +    def is_enum_val(self):
> +        return self.get_enum_name() is not None
> +
> +    def get_enum_name(self):
> +        return self.attr and self.attr.attr.get("enum")
> +

[Severity: Low]
Response parsing now supports enum selectors. Should the policy side be
addressed, or at least documented? TypeSubMessage._attr_typol() still
says:

    # Reverse-parsing of the policy (ynl_err_walk() in ynl.c) does not
    # support external selectors. No family uses sub-messages with external
    # selector for requests so this is fine for now.

Only TypeString._attr_typol() emits .is_selector = 1.
TypeScalar._attr_typol() emits only .type = YNL_PT_U<N>, so the u16
encap-type selector never gets the flag.

As a result, ynl_err_walk_is_sel() never records the selector. For any
extack offset inside RTA_ENCAP, ynl_err_walk_report_one() then reaches
this code and the attribute path is cut off at .encap:

    if (!selector) {
        if (*n < str_sz)
            *n += snprintf(str, str_sz, "(!selector)");
        return 1;
    }

For NLMSGERR_ATTR_MISS_NEST inside the encap nest, ynl_ext_ack_check()
leaves nest_pol at the outer route-attrs policy. The missing attribute
would then be named from the wrong policy.

newroute and delroute requests carry encap. The kernel does return
extack offsets inside RTA_ENCAP, for example NL_SET_ERR_MSG_ATTR on
tb[IOAM6_IPTUNNEL_*] in net/ipv6/ioam6_iptunnel.c.

ynl_err_walk_sel_policy() also assumes a string selector
(ynl_attr_get_str() plus strcmp()). It would need enum-to-string support
before is_selector could be set for scalar selectors.

The note below the commit message says this will be a follow-up. Could
the comment in _attr_typol() be updated to mention the enum/integer
selector limitation too?

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