CVE-2022-4129: Serialize access to sk_user_data with sk_callback_lock

* [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock @ 2022-11-14 19:16 Jakub Sitnicki 2022-11-15 11:12 ` Tom Parkin 2022-11-16 13:30 ` patchwork-bot+netdevbpf 0 siblings, 2 replies; 12+ messages in thread From: Jakub Sitnicki @ 2022-11-14 19:16 UTC (permalink / raw) To: netdev Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Tom Parkin, Haowei Yan

sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

v4:

• serialize write to sk_user_data in l2tp sk_destruct

v3:

• switch from sock lock to sk_callback_lock
• document write-protection for sk_user_data

v2:

• update Fixes to point to origin of the bug
• use real names in Reported/Tested-by tags

Cc: Tom Parkin [email protected] Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core") Reported-by: Haowei Yan [email protected] Signed-off-by: Jakub Sitnicki [email protected]

---

This took me forever. Sorry about that.

include/net/sock.h | 2 ± net/l2tp/l2tp_core.c | 19 ++++++++++++±----- 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h index 5db02546941c…e0517ecc6531 100644 — a/include/net/sock.h +++ b/include/net/sock.h @@ -323,7 +323,7 @@ struct sk_filter; * @sk_tskey: counter to disambiguate concurrent tstamp requests * @sk_zckey: counter to order MSG_ZEROCOPY notifications * @sk_socket: Identd and reporting IO signals - * @sk_user_data: RPC layer private data + * @sk_user_data: RPC layer private data. Write-protected by @sk_callback_lock. * @sk_frag: cached page frag * @sk_peek_off: current peek_offset value * @sk_send_head: front of stuff to transmit diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 7499c51b1850…754fdda8a5f5 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1150,8 +1150,10 @@ static void l2tp_tunnel_destruct(struct sock *sk) }

/\* Remove hooks into tunnel socket \*/

• write_lock_bh(&sk->sk_callback_lock); sk->sk_destruct = tunnel->old_sk_destruct; sk->sk_user_data = NULL;

• write_unlock_bh(&sk->sk_callback_lock);

  /* Call the original destructor */ if (sk->sk_destruct) @@ -1469,16 +1471,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, sock = sockfd_lookup(tunnel->fd, &ret); if (!sock) goto err; -

•   ret = l2tp\_validate\_socket(sock->sk, net, tunnel->encap);
  

•   if (ret < 0)
  

•       goto err\_sock;
  

  }

• sk = sock->sk;
• write_lock(&sk->sk_callback_lock);
• ret = l2tp_validate_socket(sk, net, tunnel->encap);
• if (ret < 0)

•   goto err\_sock;
  

• tunnel->l2tp_net = net; pn = l2tp_pernet(net);

- sk = sock->sk; sock_hold(sk); tunnel->sock = sk;

@@ -1504,7 +1508,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

    setup\_udp\_tunnel\_sock(net, sock, &udp\_cfg);
} else {

- sk->sk_user_data = tunnel;

•   rcu\_assign\_sk\_user\_data(sk, tunnel);
  

  }

  tunnel->old_sk_destruct = sk->sk_destruct; @@ -1518,6 +1522,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, if (tunnel->fd >= 0) sockfd_put(sock);

• write_unlock(&sk->sk_callback_lock); return 0;

err_sock: @@ -1525,6 +1530,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, sock_release(sock); else sockfd_put(sock);

• write_unlock(&sk->sk_callback_lock); err: return ret; } – 2.38.1

^ permalink raw reply related [flat|nested] 12+ messages in thread

• * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-14 19:16 [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock Jakub Sitnicki @ 2022-11-15 11:12 ` Tom Parkin 2022-11-16 13:30 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 12+ messages in thread From: Tom Parkin @ 2022-11-15 11:12 UTC (permalink / raw) To: Jakub Sitnicki Cc: netdev, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Haowei Yan

  [-- Attachment #1: Type: text/plain, Size: 3971 bytes --]

  On Mon, Nov 14, 2022 at 20:16:19 +0100, Jakub Sitnicki wrote: > sk->sk_user_data has multiple users, which are not compatible with each

  other. Writers must synchronize by grabbing the sk->sk_callback_lock.

  l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

  We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

  v4:

  • serialize write to sk_user_data in l2tp sk_destruct

  v3:

  • switch from sock lock to sk_callback_lock
  • document write-protection for sk_user_data

  v2:

  • update Fixes to point to origin of the bug
  • use real names in Reported/Tested-by tags LGTM, thanks Jakub.

  >

  Cc: Tom Parkin [email protected] Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core") Reported-by: Haowei Yan [email protected] Signed-off-by: Jakub Sitnicki [email protected]

  ---

  This took me forever. Sorry about that.

  include/net/sock.h | 2 ± net/l2tp/l2tp_core.c | 19 ++++++++++++±----- 2 files changed, 14 insertions(+), 7 deletions(-)

  diff --git a/include/net/sock.h b/include/net/sock.h index 5db02546941c…e0517ecc6531 100644 — a/include/net/sock.h +++ b/include/net/sock.h @@ -323,7 +323,7 @@ struct sk_filter; * @sk_tskey: counter to disambiguate concurrent tstamp requests * @sk_zckey: counter to order MSG_ZEROCOPY notifications * @sk_socket: Identd and reporting IO signals

  • * @sk_user_data: RPC layer private data

  • * @sk_user_data: RPC layer private data. Write-protected by @sk_callback_lock. * @sk_frag: cached page frag * @sk_peek_off: current peek_offset value * @sk_send_head: front of stuff to transmit diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 7499c51b1850…754fdda8a5f5 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1150,8 +1150,10 @@ static void l2tp_tunnel_destruct(struct sock *sk) }

  /* Remove hooks into tunnel socket */

  • write_lock_bh(&sk->sk_callback_lock); sk->sk_destruct = tunnel->old_sk_destruct; sk->sk_user_data = NULL;

  • write_unlock_bh(&sk->sk_callback_lock);

    /* Call the original destructor */ if (sk->sk_destruct) @@ -1469,16 +1471,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, sock = sockfd_lookup(tunnel->fd, &ret); if (!sock) goto err;

  • ret = l2tp\_validate\_socket(sock->sk, net, tunnel->encap);
    

  • if (ret < 0)
    

  •     goto err\_sock;
    

    }

  • sk = sock->sk;
  • write_lock(&sk->sk_callback_lock);
  • ret = l2tp_validate_socket(sk, net, tunnel->encap);
  • if (ret < 0)

  • goto err\_sock;
    

  • tunnel->l2tp_net = net; pn = l2tp_pernet(net);

  • sk = sock->sk; sock_hold(sk); tunnel->sock = sk;

  @@ -1504,7 +1508,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,

    setup\_udp\_tunnel\_sock(net, sock, &udp\_cfg);
  

  } else {

  • sk->sk\_user\_data = tunnel;
    

  • rcu\_assign\_sk\_user\_data(sk, tunnel);
    

    }

    tunnel->old_sk_destruct = sk->sk_destruct; @@ -1518,6 +1522,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, if (tunnel->fd >= 0) sockfd_put(sock);

  • write_unlock(&sk->sk_callback_lock); return 0;

  err_sock: @@ -1525,6 +1530,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, sock_release(sock); else sockfd_put(sock);

  • write_unlock(&sk->sk_callback_lock); err: return ret; } – 2.38.1

  – Tom Parkin Katalix Systems Ltd https://katalix.com Catalysts for your Embedded Linux software development

  [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]

  ^ permalink raw reply [flat|nested] 12+ messages in thread

• * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-14 19:16 [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock Jakub Sitnicki 2022-11-15 11:12 ` Tom Parkin @ 2022-11-16 13:30 ` patchwork-bot+netdevbpf 2022-11-17 9:07 ` Eric Dumazet 1 sibling, 1 reply; 12+ messages in thread From: patchwork-bot+netdevbpf @ 2022-11-16 13:30 UTC (permalink / raw) To: Jakub Sitnicki Cc: netdev, davem, edumazet, kuba, pabeni, tparkin, g1042620637

  Hello:

  This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

  On Mon, 14 Nov 2022 20:16:19 +0100 you wrote: > sk->sk_user_data has multiple users, which are not compatible with each

  other. Writers must synchronize by grabbing the sk->sk_callback_lock.

  l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

  We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

  […] Here is the summary with links: - [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

  You are awesome, thank you!

  Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html

  ^ permalink raw reply [flat|nested] 12+ messages in thread

• • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-16 13:30 ` patchwork-bot+netdevbpf @ 2022-11-17 9:07 ` Eric Dumazet 2022-11-17 9:35 ` Jakub Sitnicki 2022-11-17 9:40 ` Eric Dumazet 0 siblings, 2 replies; 12+ messages in thread From: Eric Dumazet @ 2022-11-17 9:07 UTC (permalink / raw) To: patchwork-bot+netdevbpf Cc: Jakub Sitnicki, netdev, davem, kuba, pabeni, tparkin, g1042620637

    On Wed, Nov 16, 2022 at 5:30 AM [email protected] wrote: >

    Hello:

    This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

    On Mon, 14 Nov 2022 20:16:19 +0100 you wrote:

    sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

    l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

    We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

    […]

    Here is the summary with links:

    • [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

    I guess this patch has not been tested with LOCKDEP, right ?

    sk_callback_lock always needs _bh safety.

    I will send something like:

    diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f52e4e8e2c0f47331c3b22765033d0…a3b06a3cf68248f5ec7ae8be2a9711d0f482ac36 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1474,7 +1474,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }

        sk = sock->sk;
    

    •   write\_lock(&sk->sk\_callback\_lock);
      

    •   write\_lock\_bh(&sk->sk\_callback\_lock);
      
        ret = l2tp\_validate\_socket(sk, net, tunnel->encap);
        if (ret < 0)
      

    @@ -1522,7 +1522,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, if (tunnel->fd >= 0) sockfd_put(sock);

    •   write\_unlock(&sk->sk\_callback\_lock);
      

    •   write\_unlock\_bh(&sk->sk\_callback\_lock);
        return 0;
      

    err_sock: @@ -1531,7 +1531,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, else sockfd_put(sock);

    •   write\_unlock(&sk->sk\_callback\_lock);
      

    •   write\_unlock\_bh(&sk->sk\_callback\_lock);
      

    err: return ret; }

    ^ permalink raw reply [flat|nested] 12+ messages in thread

  • • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-17 9:07 ` Eric Dumazet @ 2022-11-17 9:35 ` Jakub Sitnicki 2022-11-17 9:54 ` Eric Dumazet 2022-11-17 9:40 ` Eric Dumazet 1 sibling, 1 reply; 12+ messages in thread From: Jakub Sitnicki @ 2022-11-17 9:35 UTC (permalink / raw) To: Eric Dumazet Cc: patchwork-bot+netdevbpf, netdev, davem, kuba, pabeni, tparkin, g1042620637

      On Thu, Nov 17, 2022 at 01:07 AM -08, Eric Dumazet wrote: > On Wed, Nov 16, 2022 at 5:30 AM [email protected] wrote:

      Hello:

      This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

      On Mon, 14 Nov 2022 20:16:19 +0100 you wrote:

      sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

      l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

      We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

      […]

      Here is the summary with links:

      • [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

      I guess this patch has not been tested with LOCKDEP, right ?

      sk_callback_lock always needs _bh safety.

      I will send something like:

      diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f52e4e8e2c0f47331c3b22765033d0…a3b06a3cf68248f5ec7ae8be2a9711d0f482ac36 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1474,7 +1474,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }

          sk = sock->sk;
      

      •   write\_lock(&sk->sk\_callback\_lock);
        

      •   write\_lock\_bh(&sk->sk\_callback\_lock);
        
          ret = l2tp\_validate\_socket(sk, net, tunnel->encap);
          if (ret < 0)
        

      @@ -1522,7 +1522,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, if (tunnel->fd >= 0) sockfd_put(sock);

      •   write\_unlock(&sk->sk\_callback\_lock);
        

      •   write\_unlock\_bh(&sk->sk\_callback\_lock);
          return 0;
        

      err_sock: @@ -1531,7 +1531,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, else sockfd_put(sock);

      •   write\_unlock(&sk->sk\_callback\_lock);
        

      •   write\_unlock\_bh(&sk->sk\_callback\_lock);
        

      err: return ret; } Hmm, weird. I double checked - I have PROVE_LOCKING enabled. Didn’t see any lockdep reports when running selftests/net/l2tp.sh.

      I my defense - I thought _bh was not needed because l2tp_tunnel_register() gets called only in the process context. I mean, it’s triggered by Netlink sendmsg, but that gets processed in-line AFAIU:

      netlink_sendmsg netlink_unicast ->netlink_rcv genl_rcv genl_rcv_msg genl_family_rcv_msg genl_family_rcv_msg_doit ->doit l2tp_nl_cmd_tunnel_create l2tp_tunnel_register

      ^ permalink raw reply [flat|nested] 12+ messages in thread

    • • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-17 9:35 ` Jakub Sitnicki @ 2022-11-17 9:54 ` Eric Dumazet 0 siblings, 0 replies; 12+ messages in thread From: Eric Dumazet @ 2022-11-17 9:54 UTC (permalink / raw) To: Jakub Sitnicki Cc: patchwork-bot+netdevbpf, netdev, davem, kuba, pabeni, tparkin, g1042620637

        On Thu, Nov 17, 2022 at 1:45 AM Jakub Sitnicki [email protected] wrote: >

        On Thu, Nov 17, 2022 at 01:07 AM -08, Eric Dumazet wrote:

        On Wed, Nov 16, 2022 at 5:30 AM [email protected] wrote:

        Hello:

        This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

        On Mon, 14 Nov 2022 20:16:19 +0100 you wrote:

        sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

        l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

        We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

        […]

        Here is the summary with links:

        • [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

        I guess this patch has not been tested with LOCKDEP, right ?

        sk_callback_lock always needs _bh safety.

        I will send something like:

        diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f52e4e8e2c0f47331c3b22765033d0…a3b06a3cf68248f5ec7ae8be2a9711d0f482ac36 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1474,7 +1474,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }

            sk = sock->sk;
        

        •   write\_lock(&sk->sk\_callback\_lock);
          

        •   write\_lock\_bh(&sk->sk\_callback\_lock);
          
            ret = l2tp\_validate\_socket(sk, net, tunnel->encap);
            if (ret < 0)
          

        @@ -1522,7 +1522,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, if (tunnel->fd >= 0) sockfd_put(sock);

        •   write\_unlock(&sk->sk\_callback\_lock);
          

        •   write\_unlock\_bh(&sk->sk\_callback\_lock);
            return 0;
          

        err_sock: @@ -1531,7 +1531,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, else sockfd_put(sock);

        •   write\_unlock(&sk->sk\_callback\_lock);
          

        •   write\_unlock\_bh(&sk->sk\_callback\_lock);
          

        err: return ret; }

        Hmm, weird. I double checked - I have PROVE_LOCKING enabled. Didn’t see any lockdep reports when running selftests/net/l2tp.sh.

        I my defense - I thought _bh was not needed because l2tp_tunnel_register() gets called only in the process context. I mean, it’s triggered by Netlink sendmsg, but that gets processed in-line AFAIU:

        netlink_sendmsg netlink_unicast ->netlink_rcv genl_rcv genl_rcv_msg genl_family_rcv_msg genl_family_rcv_msg_doit ->doit l2tp_nl_cmd_tunnel_create l2tp_tunnel_register Three different syzbot reports will help to better understand the issue, sorry it is 2am for me, I am not sure in which time zone you are in …

        ^ permalink raw reply [flat|nested] 12+ messages in thread

    • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-17 9:07 ` Eric Dumazet 2022-11-17 9:35 ` Jakub Sitnicki @ 2022-11-17 9:40 ` Eric Dumazet 2022-11-17 9:55 ` Jakub Sitnicki 1 sibling, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2022-11-17 9:40 UTC (permalink / raw) To: patchwork-bot+netdevbpf Cc: Jakub Sitnicki, netdev, davem, kuba, pabeni, tparkin, g1042620637

      On Thu, Nov 17, 2022 at 1:07 AM Eric Dumazet [email protected] wrote: >

      On Wed, Nov 16, 2022 at 5:30 AM [email protected] wrote:

      Hello:

      This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

      On Mon, 14 Nov 2022 20:16:19 +0100 you wrote:

      sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

      l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

      We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

      […]

      Here is the summary with links:

      • [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

      I guess this patch has not been tested with LOCKDEP, right ?

      sk_callback_lock always needs _bh safety.

      I will send something like:

      diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f52e4e8e2c0f47331c3b22765033d0…a3b06a3cf68248f5ec7ae8be2a9711d0f482ac36 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1474,7 +1474,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }

          sk = sock->sk;
      

      •   write\_lock(&sk->sk\_callback\_lock);
        

      •   write\_lock\_bh(&sk->sk\_callback\_lock);
        

      Unfortunately this might still not work, because setup_udp_tunnel_sock->udp_encap_enable() probably could sleep in static_branch_inc() ?

      I will release the syzbot report, and let you folks work on a fix, thanks.

      ^ permalink raw reply [flat|nested] 12+ messages in thread

    • • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-17 9:40 ` Eric Dumazet @ 2022-11-17 9:55 ` Jakub Sitnicki 2022-11-18 10:28 ` Eric Dumazet 0 siblings, 1 reply; 12+ messages in thread From: Jakub Sitnicki @ 2022-11-17 9:55 UTC (permalink / raw) To: Eric Dumazet Cc: patchwork-bot+netdevbpf, netdev, davem, kuba, pabeni, tparkin, g1042620637

        On Thu, Nov 17, 2022 at 01:40 AM -08, Eric Dumazet wrote: > On Thu, Nov 17, 2022 at 1:07 AM Eric Dumazet [email protected] wrote:

        On Wed, Nov 16, 2022 at 5:30 AM [email protected] wrote:

        Hello:

        This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

        On Mon, 14 Nov 2022 20:16:19 +0100 you wrote:

        sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

        l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

        We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

        […]

        Here is the summary with links:

        • [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

        I guess this patch has not been tested with LOCKDEP, right ?

        sk_callback_lock always needs _bh safety.

        I will send something like:

        diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f52e4e8e2c0f47331c3b22765033d0…a3b06a3cf68248f5ec7ae8be2a9711d0f482ac36 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1474,7 +1474,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }

            sk = sock->sk;
        

        •   write\_lock(&sk->sk\_callback\_lock);
          

        •   write\_lock\_bh(&sk->sk\_callback\_lock);
          

        Unfortunately this might still not work, because setup_udp_tunnel_sock->udp_encap_enable() probably could sleep in static_branch_inc() ?

        I will release the syzbot report, and let you folks work on a fix, thanks. Ah, the problem is with pppol2tp_connect racing with itself. Thanks for the syzbot report. I will take a look. I live for debugging deadlocks :-)

        ^ permalink raw reply [flat|nested] 12+ messages in thread

      • • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-17 9:55 ` Jakub Sitnicki @ 2022-11-18 10:28 ` Eric Dumazet 2022-11-18 10:57 ` Jakub Sitnicki 0 siblings, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2022-11-18 10:28 UTC (permalink / raw) To: Jakub Sitnicki Cc: patchwork-bot+netdevbpf, netdev, davem, kuba, pabeni, tparkin, g1042620637

          On Thu, Nov 17, 2022 at 1:57 AM Jakub Sitnicki [email protected] wrote: >

          On Thu, Nov 17, 2022 at 01:40 AM -08, Eric Dumazet wrote:

          On Thu, Nov 17, 2022 at 1:07 AM Eric Dumazet [email protected] wrote:

          On Wed, Nov 16, 2022 at 5:30 AM [email protected] wrote:

          Hello:

          This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

          On Mon, 14 Nov 2022 20:16:19 +0100 you wrote:

          sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

          l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

          We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

          […]

          Here is the summary with links:

          • [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

          I guess this patch has not been tested with LOCKDEP, right ?

          sk_callback_lock always needs _bh safety.

          I will send something like:

          diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f52e4e8e2c0f47331c3b22765033d0…a3b06a3cf68248f5ec7ae8be2a9711d0f482ac36 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1474,7 +1474,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }

              sk = sock->sk;
          

          •   write\_lock(&sk->sk\_callback\_lock);
            

          •   write\_lock\_bh(&sk->sk\_callback\_lock);
            

          Unfortunately this might still not work, because setup_udp_tunnel_sock->udp_encap_enable() probably could sleep in static_branch_inc() ?

          I will release the syzbot report, and let you folks work on a fix, thanks.

          Ah, the problem is with pppol2tp_connect racing with itself. Thanks for the syzbot report. I will take a look. I live for debugging deadlocks :-)

          Hi Jakub, any updates on this issue ? (Other syzbot reports with the same root cause are piling up)

          Thanks

          ^ permalink raw reply [flat|nested] 12+ messages in thread

        • • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-18 10:28 ` Eric Dumazet @ 2022-11-18 10:57 ` Jakub Sitnicki 2022-11-18 11:09 ` Eric Dumazet 0 siblings, 1 reply; 12+ messages in thread From: Jakub Sitnicki @ 2022-11-18 10:57 UTC (permalink / raw) To: Eric Dumazet Cc: patchwork-bot+netdevbpf, netdev, davem, kuba, pabeni, tparkin, g1042620637

            On Fri, Nov 18, 2022 at 02:28 AM -08, Eric Dumazet wrote: > On Thu, Nov 17, 2022 at 1:57 AM Jakub Sitnicki [email protected] wrote:

            On Thu, Nov 17, 2022 at 01:40 AM -08, Eric Dumazet wrote:

            On Thu, Nov 17, 2022 at 1:07 AM Eric Dumazet [email protected] wrote:

            On Wed, Nov 16, 2022 at 5:30 AM [email protected] wrote:

            Hello:

            This patch was applied to netdev/net.git (master) by David S. Miller [email protected]:

            On Mon, 14 Nov 2022 20:16:19 +0100 you wrote:

            sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

            l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

            We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

            […]

            Here is the summary with links:

            • [net,v4] l2tp: Serialize access to sk_user_data with sk_callback_lock https://git.kernel.org/netdev/net/c/b68777d54fac

            I guess this patch has not been tested with LOCKDEP, right ?

            sk_callback_lock always needs _bh safety.

            I will send something like:

            diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f52e4e8e2c0f47331c3b22765033d0…a3b06a3cf68248f5ec7ae8be2a9711d0f482ac36 100644 — a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1474,7 +1474,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }

                sk = sock->sk;
            

            •   write\_lock(&sk->sk\_callback\_lock);
              

            •   write\_lock\_bh(&sk->sk\_callback\_lock);
              

            Unfortunately this might still not work, because setup_udp_tunnel_sock->udp_encap_enable() probably could sleep in static_branch_inc() ?

            I will release the syzbot report, and let you folks work on a fix, thanks.

            Ah, the problem is with pppol2tp_connect racing with itself. Thanks for the syzbot report. I will take a look. I live for debugging deadlocks :-)

            Hi Jakub, any updates on this issue ? (Other syzbot reports with the same root cause are piling up)

            Thanks Sorry, I don’t have anything yet. I have reserved time to work on it this afternoon (I’m in the CET timezone).

            Alternatively, I can send a revert right away and come back with fixed patch once I have that, if you prefer.

            ^ permalink raw reply [flat|nested] 12+ messages in thread

          • • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-18 10:57 ` Jakub Sitnicki @ 2022-11-18 11:09 ` Eric Dumazet 2022-11-19 13:04 ` Jakub Sitnicki 0 siblings, 1 reply; 12+ messages in thread From: Eric Dumazet @ 2022-11-18 11:09 UTC (permalink / raw) To: Jakub Sitnicki Cc: patchwork-bot+netdevbpf, netdev, davem, kuba, pabeni, tparkin, g1042620637

              On Fri, Nov 18, 2022 at 3:00 AM Jakub Sitnicki [email protected] wrote:

              >

              Sorry, I don’t have anything yet. I have reserved time to work on it this afternoon (I’m in the CET timezone).

              Alternatively, I can send a revert right away and come back with fixed patch once I have that, if you prefer. No worries, this can wait for a fix, thanks.

              ^ permalink raw reply [flat|nested] 12+ messages in thread

            • • * Re: [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock 2022-11-18 11:09 ` Eric Dumazet @ 2022-11-19 13:04 ` Jakub Sitnicki 0 siblings, 0 replies; 12+ messages in thread From: Jakub Sitnicki @ 2022-11-19 13:04 UTC (permalink / raw) To: Eric Dumazet Cc: patchwork-bot+netdevbpf, netdev, davem, kuba, pabeni, tparkin, g1042620637

                            On Fri, Nov 18, 2022 at 03:09 AM -08, Eric Dumazet wrote:
                            \> On Fri, Nov 18, 2022 at 3:00 AM Jakub Sitnicki <[email protected]> wrote:
                            >
                            >>
                            >> Sorry, I don't have anything yet. I have reserved time to work on it
                            >> this afternoon (I'm in the CET timezone).
                            >>
                            >> Alternatively, I can send a revert right away and come back with fixed
                            >> patch once I have that, if you prefer.
                            >
                            > No worries, this can wait for a fix, thanks.
                            Proposed fix now posted:
                            
                            https://lore.kernel.org/netdev/[email protected]/
                            
                            ^ permalink raw reply   \[flat|**nested**\] 12+ messages in thread

end of thread, other threads:[~2022-11-19 13:05 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) – links below jump to the message on this page – 2022-11-14 19:16 [PATCH net v4] l2tp: Serialize access to sk_user_data with sk_callback_lock Jakub Sitnicki 2022-11-15 11:12 ` Tom Parkin 2022-11-16 13:30 ` patchwork-bot+netdevbpf 2022-11-17 9:07 ` Eric Dumazet 2022-11-17 9:35 ` Jakub Sitnicki 2022-11-17 9:54 ` Eric Dumazet 2022-11-17 9:40 ` Eric Dumazet 2022-11-17 9:55 ` Jakub Sitnicki 2022-11-18 10:28 ` Eric Dumazet 2022-11-18 10:57 ` Jakub Sitnicki 2022-11-18 11:09 ` Eric Dumazet 2022-11-19 13:04 ` Jakub Sitnicki

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.