Re: [PATCH net-next 5/6] netlink: specs: rt-route: describe lwt BPF program options

[netdev-bot+sashiko]

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] Documentation/netlink/specs/rt-route.yaml re-points the
  seg6-local `bpf` attribute (SEG6_LOCAL_BPF) at the newly added `lwt-…
- [Low] The new `lwt-bpf-prog` `name` attribute in
  Documentation/netlink/specs/rt-route.yaml (lines 624-626) is declared…

--

netlink: specs: rt-route: describe lwt BPF program options

This changes the lwt-bpf in/out/xmit attributes and the seg6-local bpf
attribute in Documentation/netlink/specs/rt-route.yaml from binary to a
nest, and adds a new shared lwt-bpf-prog attribute set with fd and name
members.

> diff --git a/Documentation/netlink/specs/rt-route.yaml b/Documentation/netlink/specs/rt-route.yaml
> index 6030d7cb8d0d1..5e9537f7dfad4 100644
> --- a/Documentation/netlink/specs/rt-route.yaml
> +++ b/Documentation/netlink/specs/rt-route.yaml

[ ... ]

> @@ -503,7 +506,8 @@ attribute-sets:
>          type: u32
>        -
>          name: bpf
> -        type: binary
> +        type: nest
> +        nested-attributes: lwt-bpf-prog

[Severity: Medium]
Should the seg6-local bpf attribute point at the lwt-bpf-prog set?  This
attribute lives in the seg6-local set, which carries
header: linux/seg6_local.h, but lwt-bpf-prog below uses
name-prefix: lwt-bpf-prog-, so its members document and generate as
LWT_BPF_PROG_FD / LWT_BPF_PROG_NAME.

Those identifiers come from linux/lwtunnel.h:

    enum {
            LWT_BPF_PROG_UNSPEC,
            LWT_BPF_PROG_FD,
            LWT_BPF_PROG_NAME,
            __LWT_BPF_PROG_MAX,
    };

while the seg6-local nest is a separate uAPI namespace in
linux/seg6_local.h:

    enum {
            SEG6_LOCAL_BPF_PROG_UNSPEC,
            SEG6_LOCAL_BPF_PROG,
            SEG6_LOCAL_BPF_PROG_NAME,
            __SEG6_LOCAL_BPF_PROG_MAX,
    };

The values (1, 2) and payload types happen to coincide, so the wire format
still decodes, but does this end up documenting SRv6 attributes with
lwtunnel.h names?  There also seems to be no header: value that can be
correct for a set shared between linux/lwtunnel.h and
linux/seg6_local.h.

Would a separate seg6-local-bpf-prog set with
header: linux/seg6_local.h be more accurate here?

Separately, the commit message says only:

    Replace binary BPF attributes with a nested lwt-bpf-prog to support
    lwt bpf prog options.

Could it also mention that the seg6-local bpf attribute is being re-typed,
since that is a different attribute space?

> @@ -610,6 +614,16 @@ attribute-sets:
>        -
>          name: hwid
>          type: u8
> +  -
> +    name: lwt-bpf-prog
> +    name-prefix: lwt-bpf-prog-
> +    attributes:
> +      -
> +        name: fd
> +        type: u32

[Severity: Medium]
Is fd the right name for attribute 1 in both directions?  In requests
parse_nla_bpf() in net/ipv6/seg6_local.c does treat it as a descriptor:

    fd = nla_get_u32(tb[SEG6_LOCAL_BPF_PROG]);
    p = bpf_prog_get_type(fd, BPF_PROG_TYPE_LWT_SEG6LOCAL);

but on GET/dump put_nla_bpf() emits a program id, not a descriptor:

    if (nla_put_u32(skb, SEG6_LOCAL_BPF_PROG, slwt->bpf.prog->aux->id))
            return -EMSGSIZE;

So a spec-driven decoder would print slwt->bpf.prog->aux->id under a field
called fd, and feeding that value back into a request would resolve an
unrelated descriptor or fail.  On the lwt-bpf side bpf_fill_lwt_prog() in
net/core/lwt_bpf.c never emits attribute 1 at all and only puts
LWT_BPF_PROG_NAME.

Would it be worth naming this member after the program reference/id, or at
least documenting the id-versus-fd asymmetry?

Note this appears unchanged at the end of the series (30e3ec46f708 still
has name: bpf with nested-attributes: lwt-bpf-prog and the fd member).

> +      -
> +        name: name
> +        type: string

[Severity: Low]
Should this string carry a max-len?  Both kernel consumers of this nest
bound it to MAX_PROG_NAME (256).

net/core/lwt_bpf.c:

    static const struct nla_policy bpf_prog_policy[LWT_BPF_PROG_MAX + 1] = {
            [LWT_BPF_PROG_FD]   = { .type = NLA_U32, },
            [LWT_BPF_PROG_NAME] = { .type = NLA_NUL_STRING,
                                    .len = MAX_PROG_NAME },
    };

net/ipv6/seg6_local.c:

    static const struct nla_policy bpf_prog_policy[SEG6_LOCAL_BPF_PROG_MAX + 1] = {
            [SEG6_LOCAL_BPF_PROG]      = { .type = NLA_U32, },
            [SEG6_LOCAL_BPF_PROG_NAME] = { .type = NLA_NUL_STRING,
                                           .len = MAX_PROG_NAME },
    };

As written the spec advertises an unbounded string, so tooling generated
from it will build a request that the kernel rejects with -EINVAL.
MAX_PROG_NAME is a private define in those two .c files, so the numeric
bound would need to be spelled out in the spec, and max-len does not count
the NUL terminator.

This is also still missing at the end of the series (30e3ec46f708).

>  
>  operations:
>    enum-model: directional

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