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

Message-ID: <d828df18-64ec-9ed2-8159-ae0cb01302be@katalix.com>
Date: Fri, 2 Aug 2024 08:35:24 +0100
From: James Chapman <jchapman@...alix.com>
To: Eric Dumazet <edumazet@...gle.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 01/08/2024 16:57, Eric Dumazet wrote:
> 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;
> 
Thanks Eric. I'll submit a patch with this fix later.