Re: [PATCH net-next 1/6] net: lwtunnel: change encap fill order

[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] Incomplete rollback in lwtunnel_fill_encap()
  (net/core/lwtunnel.c:229-268): the reorder commits the selector…
- [Low] Concern (assessed INVALID by baseline verification, retained only
  for policy combination): the commit message justifies the reorder by a…

--

net: lwtunnel: change encap fill order

This moves the encap-type attribute so it is written before the encap
nest in lwtunnel_fill_encap(), so that YNL can read the sub-message
selector before the payload it selects.  No other behaviour was
intended to change.

> diff --git a/net/core/lwtunnel.c b/net/core/lwtunnel.c
> index b01a395d9a966..f13dfa02e026e 100644
> --- a/net/core/lwtunnel.c
> +++ b/net/core/lwtunnel.c
> @@ -240,6 +240,10 @@ int lwtunnel_fill_encap(struct sk_buff *skb, struct lwtunnel_state *lwtstate,
>  	    lwtstate->type > LWTUNNEL_ENCAP_MAX)
>  		return 0;
>  
> +	ret = nla_put_u16(skb, encap_type_attr, lwtstate->type);
> +	if (ret)
> +		return ret;
> +
>  	nest = nla_nest_start_noflag(skb, encap_attr);
>  	if (!nest)
>  		return -EMSGSIZE;

[Severity: Medium]
Now that the type attribute is committed to the skb before the nest is
opened, can the -EOPNOTSUPP path leave it behind?

In lwtunnel_fill_encap() the failure label only rewinds the nest, and
-EOPNOTSUPP is translated into a successful return:

net/core/lwtunnel.c:lwtunnel_fill_encap() {
	ret = -EOPNOTSUPP;
	rcu_read_lock();
	ops = rcu_dereference(lwtun_encaps[lwtstate->type]);
	if (likely(ops && ops->fill_encap))
		ret = ops->fill_encap(skb, lwtstate);
	rcu_read_unlock();

	if (ret)
		goto nla_put_failure;
	...
nla_put_failure:
	nla_nest_cancel(skb, nest);

	return (ret == -EOPNOTSUPP ? 0 : ret);
}

nla_nest_cancel() trims back only to the start of the nest, so the
encap_type_attr written just above it stays in the message.  Before this
change the type attribute was emitted only after ops->fill_encap()
succeeded, so this path left the skb unchanged.

All callers treat the 0 return as success and finish the message, for
example fib_nexthop_info():

	if (lwtunnel_fill_encap(skb, nhc->nhc_lwtstate,
				RTA_ENCAP, RTA_ENCAP_TYPE) < 0)
		goto nla_put_failure;

and the same "< 0" test appears in nh_fill_node(), rt_fill_info() and
rt6_fill_node().  Does that mean user space can now receive a route or
nexthop message carrying RTA_ENCAP_TYPE / NHA_ENCAP_TYPE with no encap
payload?

That shape is rejected by the kernel's own nexthop parser in
rtm_to_nh_config():

	} else if (tb[NHA_ENCAP_TYPE]) {
		NL_SET_ERR_MSG(extack, "LWT encapsulation attribute is missing");
		goto out;

so tools that dump and replay nexthops would see -EINVAL on restore.

Would it work to record the write position before emitting the selector
and nlmsg_trim() back to it on the failure and -EOPNOTSUPP paths, or to
reserve the selector attribute and fill it in only once the payload fill
has succeeded?

The trigger requires lwtun_encaps[lwtstate->type] to be NULL or an ops
without a ->fill_encap handler while an lwtstate of that type is still
live, which the module reference taken in lwtunnel_build_state() makes
hard to reach in normal operation, but the code explicitly tests
ops->fill_encap so the branch does not look impossible.

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