Re: [PATCH net v2 1/3] rtnetlink: return EMSGSIZE when first dump entry does not fit

[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] The concern: in rtnl_dumpit() (net/core/rtnetlink.c), this patch
  only turns an err == -EMSGSIZE returned with an empty skb into an…

--

rtnetlink: return EMSGSIZE when first dump entry does not fit

This patch changes rtnl_dumpit() for handlers registered with
RTNL_FLAG_DUMP_SPLIT_NLM_DONE. With the patch, an -EMSGSIZE return on an
empty skb is passed back to user space as an error and is no longer
dropped. Before, a first entry too large for the initial NLMSG_GOODSIZE
skb ended the dump with NLMSG_DONE and status 0.

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index e141340172ce3..2777796a4ed2c 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -7043,7 +7043,10 @@ static int rtnl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  	 * Some applications which parse netlink manually depend on this.
>  	 */
>  	if (cb->flags & RTNL_FLAG_DUMP_SPLIT_NLM_DONE) {
> -		if (err < 0 && err != -EMSGSIZE)
> +		/* -EMSGSIZE with nothing in the skb is an error, as returning
> +		 * a length of 0 would end the dump as if it was complete.
> +		 */
> +		if (err < 0 && (err != -EMSGSIZE || !skb->len))
>  			return err;

[Severity: Low]

Does this also cover the IPv6 route dump? inet6_dump_fib() is registered
with RTNL_FLAG_DUMP_SPLIT_NLM_DONE too, but it does not use -EMSGSIZE to
report that a route did not fit.

When the first route does not fit, rt6_dump_route() returns 0, and
fib6_dump_node() turns that into a positive return:

net/ipv6/ip6_fib.c:fib6_dump_node() {
    ...
		res = rt6_dump_route(rt, w->args, w->skip_in_node);
		if (res >= 0) {
			/* Frame is full, suspend walking */
			w->leaf = rt;
			...
			w->skip_in_node += res;

			return 1;
		}
    ...
}

inet6_dump_fib() passes that positive value back up. With err > 0, the
new check above never fires, and rtnl_dumpit() still reaches
"return skb->len" with skb->len == 0.

With only this patch applied, would an oversized IPv6 multipath route
still end the dump with NLMSG_DONE and status 0?

The AF_UNSPEC path has the same dependency. rtnl_dump_all() ends with
"return skb->len ? : ret", so it only reports an error if the IPv6
handler returns -EMSGSIZE.

The next patch in the series, "ipv6: return EMSGSIZE when first route in
dump does not fit", appears to close this gap. It adds the following to
inet6_dump_fib():

	if (err > 0 && !skb->len)
		err = -EMSGSIZE;

The selftest "selftests: net: netlink-dumps: test dump of route larger
than skb" also checks both AF_INET and AF_INET6.

The commit message here says in general terms that an oversized route
entry silently truncates the dump. Could it say that this patch only
fixes handlers that return -EMSGSIZE, and that the IPv6 case is fixed
by the following patch?

>  		if (!err)
>  			cb->data = NULL;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110172925.3293312-1-stephen%40networkplumber.org