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] Concern: the patch fixes rpl and ioam6 but leaves
xfrmi_build_state() (net/xfrm/xfrm_interface_core.c:83) on strict…
- [Low] Concern: the IP/IP6 tunnel encap options (LWTUNNEL_IP_OPTS /
LWTUNNEL_IP6_OPTS and the GENEVE/VXLAN/ERSPAN entries) are dumped with…
--
net: lwtunnel: accept RTA_ENCAP without NLA_F_NESTED
This adds lwtunnel_nla_parse(). It parses the attributes nested in an
lwtunnel encap strictly, but does not require NLA_F_NESTED on the encap
attribute itself. rpl and ioam6 now use it, so routes dumped by "ip route
save" can be restored with "ip route restore".
> diff --git a/include/net/lwtunnel.h b/include/net/lwtunnel.h
> index 26232f603e33c..046978d6224c2 100644
> --- a/include/net/lwtunnel.h
> +++ b/include/net/lwtunnel.h
[ ... ]
> @@ -37,6 +38,7 @@ struct lwtunnel_state {
> };
>
> struct lwtunnel_encap_ops {
> + /* encap may lack NLA_F_NESTED, parse it with lwtunnel_nla_parse() */
> int (*build_state)(struct net *net, struct nlattr *encap,
> unsigned int family, const void *cfg,
> struct lwtunnel_state **ts,
[Severity: Low]
Do other build_state callbacks still break this contract after this
patch? xfrmi_build_state() in net/xfrm/xfrm_interface_core.c still uses
the strict parser:
net/xfrm/xfrm_interface_core.c:xfrmi_build_state() {
...
ret = nla_parse_nested(tb, LWT_XFRM_MAX, nla, xfrm_lwt_policy, extack);
...
}
lwtunnel_fill_encap() emits RTA_ENCAP with the flag cleared:
net/core/lwtunnel.c:lwtunnel_fill_encap() {
...
nest = nla_nest_start_noflag(skb, encap_attr);
...
}
Would "ip route save" followed by "ip route restore" of an "encap xfrm
if_id N" route still fail with "NLA_F_NESTED is missing" at this point?
A later patch in this series, "xfrm: lwtunnel: accept RTA_ENCAP without
NLA_F_NESTED", switches xfrmi_build_state() to lwtunnel_nla_parse() and
carries its own Fixes: tag. The series as a whole closes this gap, even
though this patch does not.
> @@ -53,6 +55,31 @@ struct lwtunnel_encap_ops {
> struct module *owner;
> };
>
> +/**
> + * lwtunnel_nla_parse - parse the attributes nested in an lwtunnel encap
> + * @tb: destination array with maxtype+1 elements
> + * @maxtype: maximum attribute type to be expected
> + * @nla: encap attribute passed to &lwtunnel_encap_ops.build_state, or an
> + * attribute nested in it
> + * @policy: validation policy
> + * @extack: extended ACK report struct
> + *
> + * The encap attribute, and some of the attributes nested in it, have always
> + * been dumped without NLA_F_NESTED, and userspace such as "ip route restore"
> + * sends a dump back unchanged, so the flag cannot be required on @nla.
> + * The attributes nested in @nla are still validated strictly.
> + *
> + * Return: 0 on success or a negative error code.
> + */
> +static inline int lwtunnel_nla_parse(struct nlattr *tb[], int maxtype,
> + const struct nlattr *nla,
> + const struct nla_policy *policy,
> + struct netlink_ext_ack *extack)
> +{
> + return nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
> + extack);
> +}
[Severity: Low]
The kernel-doc says some attributes nested in the encap are also dumped
without NLA_F_NESTED. Is the IP/IP6 tunnel options path still rejected on
restore after this patch?
In net/ipv4/ip_tunnel_core.c, ip_tun_fill_encap_opts() and the
geneve/vxlan/erspan fill helpers emit the option nests without the flag:
nest = nla_nest_start_noflag(skb, type);
...
nest = nla_nest_start_noflag(skb, LWTUNNEL_IP_OPTS_GENEVE);
The input side validates them strictly in three places.
ip_tun_policy and ip6_tun_policy:
[LWTUNNEL_IP_UNSPEC] = { .strict_start_type = LWTUNNEL_IP_OPTS },
ip_tun_parse_opts():
err = nla_validate(nla_data(attr), nla_len(attr), LWTUNNEL_IP_OPTS_MAX,
...
ip_tun_parse_opts_geneve(), and the same in the vxlan and erspan parsers:
err = nla_parse_nested(tb, LWTUNNEL_IP_OPT_GENEVE_MAX, attr,
...
lwtunnel_nla_parse() calls nla_parse(), which uses NL_VALIDATE_STRICT. If
a policy had NLA_NESTED children without the flag, it would reject them
too. Could the kernel-doc sentence "The attributes nested in @nla are
still validated strictly" mislead callers that expect unflagged nested
children to be accepted?
This restore failure for the tunnel options has existed since
ed02551f58b9. This patch does not change it.
A later patch in this series, "net: ip_tunnel: accept tunnel options
without NLA_F_NESTED", makes three changes:
- It moves strict_start_type to LWTUNNEL_IP(6)_OPTS + 1.
- It adds lwtunnel_nla_validate() and uses it in ip_tun_parse_opts().
- It switches the geneve/vxlan/erspan parsers to lwtunnel_nla_parse().
None of the policies passed to lwtunnel_nla_parse() in the series have
NLA_NESTED entries. In practice, its strict mode never rejects an
unflagged child.
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-lwt-encap-noflag-v1-0-8de7ab6c86e9%40gmail.com
