netdev - Re: RFC: l2tp: how to fix nested socket lockdep splat

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJkFh4JmNO2gM-4c6sbqgdjFzdNZUc-b6jupTMpUaC1mQ@mail.gmail.com>
Date: Thu, 1 Aug 2024 17:57:44 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: James Chapman <jchapman@...alix.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, jakub@...udflare.com, 
	tparkin@...alix.com
Subject: Re: RFC: l2tp: how to fix nested socket lockdep splat

On Thu, Aug 1, 2024 at 5:27 PM James Chapman <jchapman@...alix.com> wrote:
>
> When l2tp tunnels use a socket provided by userspace, we can hit lockdep splats like the below when data is transmitted through another (unrelated) userspace socket which then gets routed over l2tp.
>
> This issue was previously discussed here: https://lore.kernel.org/netdev/87sfialu2n.fsf@cloudflare.com/
>
> Is it reasonable to disable lockdep tracking of l2tp userspace tunnel sockets? Is there a better solution?
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index e22e2a45c925..ab7be05da7d4 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1736,6 +1736,16 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>         }
>
>         sk->sk_allocation = GFP_ATOMIC;
> +
> +       /* If the tunnel socket is a userspace socket, disable lockdep
> +        * validation on the tunnel socket to avoid lockdep splats caused by
> +        * nested socket calls on the same lockdep socket class. This can
> +        * happen when data from a user socket is routed over l2tp, which uses
> +        * another userspace socket.
> +        */
> +       if (tunnel->fd >= 0)
> +               lockdep_set_novalidate_class(&sk->sk_lock.slock);
> +


I would rather use

// Must be different than SINGLE_DEPTH_NESTING
#define L2TP_DEPTH_NESTED 2

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 5d2068b6c77859c0e2e3166afd8e2e1e32512445..4d747a0cf30c645e51c27e531d23db682259155f
100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1171,7 +1171,7 @@ static int l2tp_xmit_core(struct l2tp_session
*session, struct sk_buff *skb, uns
        IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE |
IPSKB_XFRM_TRANSFORMED | IPSKB_REROUTED);
        nf_reset_ct(skb);

-       bh_lock_sock_nested(sk);
+       spin_lock_nested(&sk->sk_lock.slock, L2TP_DEPTH_NESTING);
        if (sock_owned_by_user(sk)) {
                kfree_skb(skb);
                ret = NET_XMIT_DROP;