Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock

From: Hangbin Liu <liuhangbin@gmail.com>
To: Jakub Sitnicki <jakub@cloudflare.com>
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Tom Parkin <tparkin@katalix.com>,
	Haowei Yan <g1042620637@gmail.com>,
	Roopa Prabhu <roopa@nvidia.com>,
	Nikolay Aleksandrov <nikolay@nvidia.com>
Subject: Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock
Date: Mon, 5 Dec 2022 20:37:57 +0800	[thread overview]
Message-ID: <Y43mJUHWE5+9t2Ak@Laptop-X1> (raw)
In-Reply-To: <87y1rmhyc2.fsf@cloudflare.com>

On Mon, Dec 05, 2022 at 11:24:39AM +0100, Jakub Sitnicki wrote:
> > Hi Jakub,
> >
> > I have a similar issue with vxlan driver. Similar with commit
> > ad6c9986bcb6 ("vxlan: Fix GRO cells race condition between receive and link
> > delete"). There is still a race condition on vxlan that when receive a packet
> > while deleting a VXLAN device. In vxlan_ecn_decapsulate(), the
> > vxlan_get_sk_family() call panic as sk is NULL.
> >
> > So I'm wondering if we should also have locks in udp_tunnel_sock_release().
> > Or should we add a checking in sk state before calling vxlan_get_sk_family()?
> 
> This is how like to think about it:
> 
> To know when it is safe to load vs->sock->sk->sk_family, we have to ask:
> 
> 1. What ensures that the objects remain alive/valid in our scope?
> 2. What protects the objects from being mutated?
> 
> In case of vxlan_sock object in the context of vxlan_ecn_decapsulate():
> 
> 1. We are in an RCU read side section (ip_local_deliver_finish).
> 2. RCU-protected objects are not to be mutated while readers exist.
> 
> The classic "What is RCU, Fundamentally?" article explains it much
> better than I ever could:
> 
> https://lwn.net/Articles/262464/
> 
> As to where the problem lies. I belive udp_tunnel_sock_release() is not
> keeping the (2) promise.
> 
> After unpublishing the sk_user_data, we should wait for any existing
> readers accessing the vxlan_sock to finish with synchronize_rcu(),
> before releaseing the socket.
> 
> That is:
> 
> --- a/net/ipv4/udp_tunnel_core.c
> +++ b/net/ipv4/udp_tunnel_core.c
> @@ -176,6 +176,7 @@ EXPORT_SYMBOL_GPL(udp_tunnel_xmit_skb);
>  void udp_tunnel_sock_release(struct socket *sock)
>  {
>         rcu_assign_sk_user_data(sock->sk, NULL);
> +       synchronize_rcu();
>         kernel_sock_shutdown(sock, SHUT_RDWR);
>         sock_release(sock);
>  }
> 
> 
> Otherwise accessing vxlan_sock state doesn't look safe to me.

Hi Jakub,

Thanks for your explain. As it's a little on my side, I will read your
comments and try your suggestion tomorrow. Currently, I use the following
draft patch to fix the vxlan issue.

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 2122747a0224..53259b0b07f3 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -4234,6 +4234,7 @@ static void vxlan_dellink(struct net_device *dev, struct list_head *head)
        struct vxlan_dev *vxlan = netdev_priv(dev);

        vxlan_flush(vxlan, true);
+       vxlan_sock_release(vxlan);

        list_del(&vxlan->next);
        unregister_netdevice_queue(dev, head);


Cheers
Hangbin
