netdev - Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y43mJUHWE5+9t2Ak@Laptop-X1>
Date:   Mon, 5 Dec 2022 20:37:57 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Tom Parkin <tparkin@...alix.com>,
        Haowei Yan <g1042620637@...il.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <nikolay@...dia.com>
Subject: Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with
 sk_callback_lock

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