Re: [PATCH v1 net] ipv6: Fix dst leak for uncached routes.

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (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 backport metadata: the Fixes: tags select stable
  trees where the patch is either useless or actively harmful, and the…
- [Low] Comment vs. code mismatch left by the patch.

Pre-existing issues:
- [Low] Documented refcount contract of the IPv6 link-scope l3mdev lookup
  disagrees with the implementation.

--

ipv6: Fix dst leak for uncached routes.

This replaces the list_empty(&rt->dst.rt_uncached) test in
ip6_route_output_flags(), ip6_rt_put_flags() and ip6_dst_check() with a
check of rt->dst.rt_uncached_list, so a route that was already unlinked
from rt6_uncached_list by rt6_uncached_list_flush_dev() is still treated
as holding the reference taken by ip6_rt_cache_alloc().

A couple of questions about the Fixes: tags and about the comments that
were left behind.

> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Fixes: d64a1f574a29 ("ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic")
> Fixes: a4c2fd7f7891 ("net: remove DST_NOCACHE flag")

[Severity: Medium]
Should this also carry Fixes: 98aa546af5e4 ("inet: remove (struct
uncached_list)->quarantine")?

The commit message itself says the routes used to be moved with
list_move() and are only unlinked with list_del_init() since that
commit.  Before it, rt6_uncached_list_flush_dev() left the entry linked
on ul->quarantine, so list_empty(&rt->dst.rt_uncached) could not become
true for a live uncached route.  The other unlinker,
rt6_uncached_list_del(), only runs from ip6_dst_destroy() and
xfrm6_dst_destroy(), i.e. after the refcount already dropped to zero.

The three tags listed point at 2018/2019 commits, which selects
v4.20..v6.10 trees where the misdetection described here cannot happen.

There also seems to be an undisclosed dependency worth a note below the
--- line.  The new predicate uses dst.rt_uncached_list as a "was ever
added to an uncached list" flag, which only works because 9a6f0c4d5796
("dst: fix races in rt6_uncached_list_del() and rt_del_uncached_list()")
added the initialization in dst_init():

net/core/dst.c:dst_init() {
	...
	INIT_LIST_HEAD(&dst->rt_uncached);
	dst->rt_uncached_list = NULL;
	...
}

dst_alloc() uses kmem_cache_alloc() without __GFP_ZERO, so on a tree
without 9a6f0c4d5796 the field is slab garbage for any dst never placed
on an uncached list.  There, would ip6_route_output_flags() skip
dst_hold_safe() and return an unreferenced dst, and would
ip6_rt_put_flags() call ip6_rt_put() on a dst it never referenced?

On top of that, dst.rt_uncached_list only exists since d288a162dd1c
(v6.4); before that the field lived in rt6_info as rt6i_uncached_list,
so the patch would not build on the older trees the tags select.

> diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
> index b9e8d2b759e9..2c5ded121f54 100644
> --- a/include/net/ip6_route.h
> +++ b/include/net/ip6_route.h
> @@ -106,7 +106,7 @@ static inline struct dst_entry *ip6_route_output(struct net *net,
>  static inline void ip6_rt_put_flags(struct rt6_info *rt, int flags)
>  {
>  	if (!(flags & RT6_LOOKUP_F_DST_NOREF) ||
> -	    !list_empty(&rt->dst.rt_uncached))
> +	    rt->dst.rt_uncached_list)
>  		ip6_rt_put(rt);
>  }

[Severity: Low]
Should the comment just above ip6_rt_put_flags() be updated too?  It
still describes list membership:

include/net/ip6_route.h
/* Only conditionally release dst if flags indicates
 * !RT6_LOOKUP_F_DST_NOREF or dst is in uncached_list.
 */

rt->dst.rt_uncached_list is assigned in rt6_uncached_list_add() and is
never reset to NULL; both rt6_uncached_list_del() and
rt6_uncached_list_flush_dev() only list_del_init() the node.  So after
this change the test means "was ever added to an uncached list", which is
intentionally still true for routes already unlinked by device flushing.

Leaving the old wording in place makes it easy for someone to later
"restore" the list_empty() form and bring the leak back.

> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 08bd68f1b5bb..b18cd0d9148c 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2722,7 +2722,7 @@ struct dst_entry *ip6_route_output_flags(struct net *net,
>  	dst = ip6_route_output_flags_noref(net, sk, fl6, flags);
>  	rt6 = dst_rt6_info(dst);
>  	/* For dst cached in uncached_list, refcnt is already taken. */
> -	if (list_empty(&rt6->dst.rt_uncached) && !dst_hold_safe(dst)) {
> +	if (!rt6->dst.rt_uncached_list && !dst_hold_safe(dst)) {

[Severity: Low]
Same question here for the comment sitting directly on the converted
line, "For dst cached in uncached_list, refcnt is already taken."  The
condition no longer asks whether the dst is currently on the list, and
the flushed-but-still-referenced case is exactly the one this fix
depends on.  Would "for dst that was added to uncached_list" (or
similar) be more accurate?

[Severity: Low]
This is a pre-existing inconsistency and not something introduced here,
but since this is the site that compensates for it: the documented
refcount contract of the l3mdev link-scope lookup disagrees with what
the implementation can return.

net/l3mdev/l3mdev.c, kernel-doc for l3mdev_link_scope_lookup():
 *	This function does not hold refcnt on the returned dst.
 *	Caller must hold rcu_read_lock().

drivers/net/vrf.c, above vrf_link_scope_lookup():
 * Note: Caller to this function must hold rcu_read_lock() and no refcnt
 * is taken on the dst by this function.

But vrf_link_scope_lookup() -> vrf_ip6_route_lookup() -> ip6_pol_route()
passes RT6_LOOKUP_F_DST_NOREF, and the FLOWI_FLAG_KNOWN_NH branch of
ip6_pol_route() states the opposite rule:

net/ipv6/route.c:ip6_pol_route() {
	...
			/* 1 refcnt is taken during ip6_rt_cache_alloc().
			 * As rt6_uncached_list_add() does not consume refcnt,
			 * this refcnt is always returned to the caller even
			 * if caller sets RT6_LOOKUP_F_DST_NOREF flag.
			 */
			rt6_uncached_list_add(rt);
	...
}

That branch looks reachable from this path: an IPv6 raw socket with
IPV6_HDRINCL sets FLOWI_FLAG_KNOWN_NH, and with a link-local or
multicast destination and an oif enslaved to a VRF,
ip6_route_output_flags_noref() goes through l3mdev_link_scope_lookup();
if the matched nexthop has no gateway the RTF_CACHE clone is returned
ref-held.

No leak today, because ip6_route_output_flags() is the only caller and
encodes the exception in the line this patch changes.  Would it be worth
correcting the exported kernel-doc and the vrf comment, so a future
l3mdev_link_scope_lookup() caller does not drop that reference and leak
the dst along with the device pinned via rt->from?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918041439.2575935-1-kuniyu%40google.com