All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ido Schimmel <idosch@nvidia.com>
To: Kuniyuki Iwashima <kuniyu@google.com>
Cc: David Ahern <dsahern@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Simon Horman <horms@kernel.org>, Wei Wang <weiwan@google.com>,
	Marc Harvey <marcharvey@google.com>,
	Kuniyuki Iwashima <kuni1840@gmail.com>,
	netdev@vger.kernel.org
Subject: Re: [PATCH v1 net] ipv6: Fix dst leak for uncached routes.
Date: Sun, 20 Sep 2026 20:26:28 +0300	[thread overview]
Message-ID: <20260920172628.GA2051643@shredder> (raw)
In-Reply-To: <20260918041439.2575935-1-kuniyu@google.com>

On Fri, Sep 18, 2026 at 04:14:37AM +0000, Kuniyuki Iwashima wrote:
> ip6_route_output_flags(), ip6_rt_put_flags(), and ip6_dst_check()
> detect an uncached route by list_empty(&rt->dst.rt_uncached),
> which replaced the static DST_NOCACHE flag check in commit
> a4c2fd7f7891 ("net: remove DST_NOCACHE flag").
> 
> When a device is unregistered, rt6_uncached_list_flush_dev()
> unlinks uncached routes tied to the device from rt6_uncached_list.
> 
> Previously, they were moved to another list with list_move()
> (__list_del_entry() + list_add()), and since commit 98aa546af5e4
> ("inet: remove (struct uncached_list)->quarantine"), the routes
> are just unlinked with list_del_init().
> 
> If list_del_init() runs concurrently, list_empty() evaluates to
> true; ip6_route_output_flags() calls dst_hold_safe() incorrectly
> and ip6_rt_put_flags() skips ip6_rt_put(), leaking dst, and thus
> dev tied via rt->from as well.
> 
> The same race is partially fixed by commit 9a6f0c4d5796 ("dst:
> fix races in rt6_uncached_list_del() and rt_del_uncached_list()").
> 
> Let's check rt6->dst.rt_uncached_list instead.

Please add a comment that we now rely on this field being written once (never
NULL-ed). Otherwise it's quite fragile. Something like:

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 36e999b22d42..a6b21edbda29 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -139,6 +139,7 @@ void rt6_uncached_list_add(struct rt6_info *rt)
 {
 	struct uncached_list *ul = raw_cpu_ptr(&rt6_uncached_list);
 
+	/* Set once and never cleared: non-NULL marks an uncached route. */
 	rt->dst.rt_uncached_list = ul;
 
 	spin_lock_bh(&ul->lock);

> 
> Note that IPv4 does not have the same issue.
> 
> 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")

The commit message reads as if the problem is only visible since
98aa546af5e4 (v6.11), so why blame commits going back to v4.13?

> Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
> ---
>  include/net/ip6_route.h | 2 +-
>  net/ipv6/route.c        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> 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);
>  }
>  
> 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)) {
>  		dst = &net->ipv6.ip6_null_entry->dst;
>  		dst_hold(dst);
>  	}

Nit: The comments here and above ip6_rt_put_flags() are now stale given
that we no longer check for membership in uncached_list. You can change
to something like:

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 2c5ded121f54..0f9b7a260d25 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -101,7 +101,7 @@ static inline struct dst_entry *ip6_route_output(struct net *net,
 }
 
 /* Only conditionally release dst if flags indicates
- * !RT6_LOOKUP_F_DST_NOREF or dst is in uncached_list.
+ * !RT6_LOOKUP_F_DST_NOREF or dst is uncached.
  */
 static inline void ip6_rt_put_flags(struct rt6_info *rt, int flags)
 {
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 36e999b22d42..515115abe2f0 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2721,7 +2721,7 @@ struct dst_entry *ip6_route_output_flags(struct net *net,
 	rcu_read_lock();
 	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. */
+	/* For an uncached dst, refcnt is already taken. */
 	if (!rt6->dst.rt_uncached_list && !dst_hold_safe(dst)) {
 		dst = &net->ipv6.ip6_null_entry->dst;
 		dst_hold(dst);

  parent reply	other threads:[~2026-09-20 17:26 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-18  4:14 [PATCH v1 net] ipv6: Fix dst leak for uncached routes Kuniyuki Iwashima
2026-09-20  2:36 ` Hangbin Liu
2026-09-20  7:39 ` Xuanqiang Luo
2026-09-20 17:26 ` Ido Schimmel [this message]
2026-09-20 19:01   ` Kuniyuki Iwashima
2026-09-21  4:17 ` netdev-bot+sashiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260920172628.GA2051643@shredder \
    --to=idosch@nvidia.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=kuba@kernel.org \
    --cc=kuni1840@gmail.com \
    --cc=kuniyu@google.com \
    --cc=marcharvey@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=weiwan@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.