From: Jakub Sitnicki <jakub@cloudflare•com>
To: Eric Dumazet <edumazet@google•com>,
Tetsuo Handa <penguin-kernel@I-love•SAKURA.ne.jp>
Cc: Boqun Feng <boqun.feng@gmail•com>,
"David S. Miller" <davem@davemloft•net>,
Jakub Kicinski <kuba@kernel•org>, Paolo Abeni <pabeni@redhat•com>,
Peter Zijlstra <peterz@infradead•org>,
Ingo Molnar <mingo@redhat•com>, Will Deacon <will@kernel•org>,
Waiman Long <longman@redhat•com>,
netdev@vger•kernel.org,
syzbot <syzbot+94cc2a66fc228b23f360@syzkaller•appspotmail.com>,
syzkaller-bugs@googlegroups•com
Subject: Re: WARNING: locking bug in inet_autobind
Date: Tue, 22 Nov 2022 19:02:31 +0100 [thread overview]
Message-ID: <87sfialu2n.fsf@cloudflare.com> (raw)
In-Reply-To: <c9695548-3f27-dda1-3124-ec21da106741@I-love.SAKURA.ne.jp>
On Tue, Sep 27, 2022 at 10:00 PM +09, Tetsuo Handa wrote:
> On 2022/09/19 14:02, Tetsuo Handa wrote:
>> But unfortunately reordering
>>
>> tunnel->sock = sk;
>> ...
>> lockdep_set_class_and_name(&sk->sk_lock.slock,...);
>>
>> by
>>
>> lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class, "l2tp_sock");
>> smp_store_release(&tunnel->sock, sk);
>>
>> does not help, for connect() on AF_INET6 socket is not finding this "sk" by
>> accessing tunnel->sock.
>>
>
> I considered something like below diff, but I came to think that this problem
> cannot be solved unless l2tp_tunnel_register() stops using userspace-supplied
> file descriptor and starts always calling l2tp_tunnel_sock_create(), for
> userspace can continue using userspace-supplied file descriptor as if a normal
> socket even after lockdep_set_class_and_name() told that this is a tunneling
> socket.
>
> Since userspace-supplied file descriptor has to be a datagram socket,
> can we somehow copy the source/destination addresses from
> userspace-supplied socket to kernel-created socket?
>
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 7499c51b1850..07429bed7c4c 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1382,8 +1382,6 @@ static int l2tp_tunnel_sock_create(struct net *net,
> return err;
> }
>
> -static struct lock_class_key l2tp_socket_class;
> -
> int l2tp_tunnel_create(int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
> struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp)
> {
> @@ -1509,8 +1507,20 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
>
> tunnel->old_sk_destruct = sk->sk_destruct;
> sk->sk_destruct = &l2tp_tunnel_destruct;
> - lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class,
> - "l2tp_sock");
> + if (IS_ENABLED(CONFIG_LOCKDEP)) {
> + static struct lock_class_key l2tp_socket_class;
> +
> + /* Changing class/name of an already visible sock might race
> + * with first lock_sock() call on that sock. In order to make
> + * sure that register_lock_class() has completed before
> + * lockdep_set_class_and_name() changes class/name, explicitly
> + * lock/release that sock.
> + */
> + lock_sock(sk);
> + release_sock(sk);
> + lockdep_set_class_and_name(&sk->sk_lock.slock,
> + &l2tp_socket_class, "l2tp_sock");
> + }
> sk->sk_allocation = GFP_ATOMIC;
>
> trace_register_tunnel(tunnel);
What if we revisit Eric's lockdep splat fix in 37159ef2c1ae ("l2tp: fix
a lockdep splat") and:
1. remove the lockdep_set_class_and_name(...) call in l2tp; it looks
like an odd case within the network stack, and
2. switch to bh_lock_sock_nested in l2tp_xmit_core so that we don't
break what has been fixed in 37159ef2c1ae.
Eric, WDYT?
next prev parent reply other threads:[~2022-11-22 18:31 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-16 5:46 WARNING: locking bug in inet_autobind syzbot
2019-05-21 8:31 ` syzbot
2019-05-22 3:16 ` syzbot
2022-09-18 15:52 ` Tetsuo Handa
2022-09-18 18:25 ` Boqun Feng
2022-09-19 5:02 ` Tetsuo Handa
2022-09-27 13:00 ` Tetsuo Handa
2022-11-22 18:02 ` Jakub Sitnicki [this message]
2022-12-29 6:26 ` [syzbot] " syzbot
2023-01-03 15:39 ` Felix Kuehling
2023-01-03 16:05 ` Waiman Long
2023-01-03 16:20 ` Felix Kuehling
2023-01-03 22:07 ` Tetsuo Handa
2023-01-03 22:12 ` Eric Dumazet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87sfialu2n.fsf@cloudflare.com \
--to=jakub@cloudflare$(echo .)com \
--cc=boqun.feng@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=longman@redhat$(echo .)com \
--cc=mingo@redhat$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=penguin-kernel@I-love$(echo .)SAKURA.ne.jp \
--cc=peterz@infradead$(echo .)org \
--cc=syzbot+94cc2a66fc228b23f360@syzkaller$(echo .)appspotmail.com \
--cc=syzkaller-bugs@googlegroups$(echo .)com \
--cc=will@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox