Re: [Patch net v3 2/2] l2tp: close all race conditions in l2tp_tunnel_register()

[Guillaume Nault <gnault@redhat.com>]

On Tue, Jan 17, 2023 at 09:10:20AM +0100, Eric Dumazet wrote:
> On Tue, Jan 17, 2023 at 9:08 AM Eric Dumazet <edumazet@google.com> wrote:
> > On Sat, Jan 14, 2023 at 4:01 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >  int l2tp_tunnel_create(int fd, int version, u32 tunnel_id, u32 peer_tunnel_id,
> > >                        struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp)
> > >  {
> > > @@ -1482,21 +1480,16 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
> > >         }
> > >
> > >         sk = sock->sk;
> > > +       lock_sock(sk);
> > >         write_lock_bh(&sk->sk_callback_lock);
> > >         ret = l2tp_validate_socket(sk, net, tunnel->encap);
> > > -       if (ret < 0)
> > > +       if (ret < 0) {
> >
> > I think we need to write_unlock_bh(&sk->sk_callback_lock)
> > before release_sock(), or risk lockdep reports.
> >
> 
> Any objection if I propose :
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index b6554e32bb12ae7813cc06c01e4d1380af667375..03608d3ded4b83d1e59e064e482f54cffcdf5240
> 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1483,10 +1483,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel
> *tunnel, struct net *net,
>         lock_sock(sk);
>         write_lock_bh(&sk->sk_callback_lock);
>         ret = l2tp_validate_socket(sk, net, tunnel->encap);
> -       if (ret < 0) {
> -               release_sock(sk);
> +       if (ret < 0)
>                 goto err_inval_sock;
> -       }
>         rcu_assign_sk_user_data(sk, tunnel);
>         write_unlock_bh(&sk->sk_callback_lock);
> 
> @@ -1523,6 +1521,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel
> *tunnel, struct net *net,
> 
>  err_inval_sock:
>         write_unlock_bh(&sk->sk_callback_lock);
> +       release_sock(sk);
> 
>         if (tunnel->fd < 0)
>                 sock_release(sock);

Indeed, that looks more correct.
Thanks!