[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240708185535.96845-1-kuniyu@amazon.com>
Date: Mon, 8 Jul 2024 11:55:35 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <edumazet@...gle.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <kuba@...nel.org>,
<kuni1840@...il.com>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>,
<pabeni@...hat.com>, <syzkaller@...glegroups.com>,
<willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH v1 net] udp: Set SOCK_RCU_FREE earlier in udp_lib_get_port().
From: Eric Dumazet <edumazet@...gle.com>
Date: Mon, 8 Jul 2024 11:38:41 -0700
> On Mon, Jul 8, 2024 at 11:20 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> >
> > syzkaller triggered the warning [0] in udp_v4_early_demux().
> >
> > In udp_v4_early_demux(), we do not touch the refcount of the looked-up
> > sk and use sock_pfree() as skb->destructor, so we check SOCK_RCU_FREE
> > to ensure that the sk is safe to access during the RCU grace period.
> >
> > Currently, SOCK_RCU_FREE is flagged for a bound socket after being put
> > into the hash table. Moreover, the SOCK_RCU_FREE check is done too
> > early in udp_v4_early_demux(), so there could be a small race window:
> >
> > CPU1 CPU2
> > ---- ----
> > udp_v4_early_demux() udp_lib_get_port()
> > | |- hlist_add_head_rcu()
> > |- sk = __udp4_lib_demux_lookup() |
> > |- DEBUG_NET_WARN_ON_ONCE(sk_is_refcounted(sk));
> > `- sock_set_flag(sk, SOCK_RCU_FREE)
> >
> > In practice, sock_pfree() is called much later, when SOCK_RCU_FREE
> > is most likely propagated for other CPUs; otherwise, we will see
> > another warning of sk refcount underflow, but at least I didn't.
> >
> > Technically, moving sock_set_flag(sk, SOCK_RCU_FREE) before
> > hlist_add_{head,tail}_rcu() does not guarantee the order, and we
> > must put smp_mb() between them, or smp_wmb() there and smp_rmb()
> > in udp_v4_early_demux().
> >
> > But it's overkill in the real scenario, so I just put smp_mb() only under
> > CONFIG_DEBUG_NET to silence the splat. When we see the refcount underflow
> > warning, we can remove the config guard.
> >
> > Another option would be to remove DEBUG_NET_WARN_ON_ONCE(), but this could
> > make future debugging harder without the hints in udp_v4_early_demux() and
> > udp_lib_get_port().
> >
> > [0]:
> >
> > Fixes: 08842c43d016 ("udp: no longer touch sk->sk_refcnt in early demux")
> > Reported-by: syzkaller <syzkaller@...glegroups.com>
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > ---
> > net/ipv4/udp.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 189c9113fe9a..1a05cc3d2b4f 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -326,6 +326,12 @@ int udp_lib_get_port(struct sock *sk, unsigned short snum,
> > goto fail_unlock;
> > }
> >
> > + sock_set_flag(sk, SOCK_RCU_FREE);
>
> Nice catch.
>
> > +
> > + if (IS_ENABLED(CONFIG_DEBUG_NET))
> > + /* for DEBUG_NET_WARN_ON_ONCE() in udp_v4_early_demux(). */
> > + smp_mb();
> > +
>
> I do not think this smp_mb() is needed. If this was, many other RCU
> operations would need it,
>
> RCU rules mandate that all memory writes must be committed before the
> object can be seen
> by other cpus in the hash table.
>
> This includes the setting of the SOCK_RCU_FREE flag.
>
> For instance, hlist_add_head_rcu() does a
> rcu_assign_pointer(hlist_first_rcu(h), n);
Ah, I was thinking spinlock will not prevent reordering, but
now I see, rcu_assign_pointer() had necessary barrier. :)
/**
* rcu_assign_pointer() - assign to RCU-protected pointer
...
* Assigns the specified value to the specified RCU-protected
* pointer, ensuring that any concurrent RCU readers will see
* any prior initialization.
will remove smp_mb() and update the changelog in v2.
Thanks !
Powered by blists - more mailing lists