lkml.org 
[lkml]   [2023]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH v2] l2tp: Avoid possible recursive deadlock in l2tp_tunnel_register()
    Date
    When a file descriptor of pppol2tp socket is passed as file descriptor
    of UDP socket, a recursive deadlock occurs in l2tp_tunnel_register().
    This situation is reproduced by the following program:

    int main(void)
    {
    int sock;
    struct sockaddr_pppol2tp addr;

    sock = socket(AF_PPPOX, SOCK_DGRAM, PX_PROTO_OL2TP);
    if (sock < 0) {
    perror("socket");
    return 1;
    }

    addr.sa_family = AF_PPPOX;
    addr.sa_protocol = PX_PROTO_OL2TP;
    addr.pppol2tp.pid = 0;
    addr.pppol2tp.fd = sock;
    addr.pppol2tp.addr.sin_family = PF_INET;
    addr.pppol2tp.addr.sin_port = htons(0);
    addr.pppol2tp.addr.sin_addr.s_addr = inet_addr("192.168.0.1");
    addr.pppol2tp.s_tunnel = 1;
    addr.pppol2tp.s_session = 0;
    addr.pppol2tp.d_tunnel = 0;
    addr.pppol2tp.d_session = 0;

    if (connect(sock, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
    perror("connect");
    return 1;
    }

    return 0;
    }

    This program causes the following lockdep warning:

    ============================================
    WARNING: possible recursive locking detected
    6.2.0-rc5-00205-gc96618275234 #56 Not tainted
    --------------------------------------------
    repro/8607 is trying to acquire lock:
    ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: l2tp_tunnel_register+0x2b7/0x11c0

    but task is already holding lock:
    ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: pppol2tp_connect+0xa82/0x1a30

    other info that might help us debug this:
    Possible unsafe locking scenario:

    CPU0
    ----
    lock(sk_lock-AF_PPPOX);
    lock(sk_lock-AF_PPPOX);

    *** DEADLOCK ***

    May be due to missing lock nesting notation

    1 lock held by repro/8607:
    #0: ffff8880213c8130 (sk_lock-AF_PPPOX){+.+.}-{0:0}, at: pppol2tp_connect+0xa82/0x1a30

    stack backtrace:
    CPU: 0 PID: 8607 Comm: repro Not tainted 6.2.0-rc5-00205-gc96618275234 #56
    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.1-2.fc37 04/01/2014
    Call Trace:
    <TASK>
    dump_stack_lvl+0x100/0x178
    __lock_acquire.cold+0x119/0x3b9
    ? lockdep_hardirqs_on_prepare+0x410/0x410
    lock_acquire+0x1e0/0x610
    ? l2tp_tunnel_register+0x2b7/0x11c0
    ? lock_downgrade+0x710/0x710
    ? __fget_files+0x283/0x3e0
    lock_sock_nested+0x3a/0xf0
    ? l2tp_tunnel_register+0x2b7/0x11c0
    l2tp_tunnel_register+0x2b7/0x11c0
    ? sprintf+0xc4/0x100
    ? l2tp_tunnel_del_work+0x6b0/0x6b0
    ? debug_object_deactivate+0x320/0x320
    ? lockdep_init_map_type+0x16d/0x7a0
    ? lockdep_init_map_type+0x16d/0x7a0
    ? l2tp_tunnel_create+0x2bf/0x4b0
    ? l2tp_tunnel_create+0x3c6/0x4b0
    pppol2tp_connect+0x14e1/0x1a30
    ? pppol2tp_put_sk+0xd0/0xd0
    ? aa_sk_perm+0x2b7/0xa80
    ? aa_af_perm+0x260/0x260
    ? bpf_lsm_socket_connect+0x9/0x10
    ? pppol2tp_put_sk+0xd0/0xd0
    __sys_connect_file+0x14f/0x190
    __sys_connect+0x133/0x160
    ? __sys_connect_file+0x190/0x190
    ? lockdep_hardirqs_on+0x7d/0x100
    ? ktime_get_coarse_real_ts64+0x1b7/0x200
    ? ktime_get_coarse_real_ts64+0x147/0x200
    ? __audit_syscall_entry+0x396/0x500
    __x64_sys_connect+0x72/0xb0
    do_syscall_64+0x38/0xb0
    entry_SYSCALL_64_after_hwframe+0x63/0xcd

    This patch fixes the issue by getting/creating the tunnel before
    locking the pppol2tp socket.

    Fixes: 0b2c59720e65 ("l2tp: close all race conditions in l2tp_tunnel_register()")
    Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
    ---
    v2: Get/create the tunnel before locking the pppol2tp socket suggested
    by Guillaume Nault.
    v1: https://lore.kernel.org/all/20230130154438.1373750-1-syoshida@redhat.com/

    net/l2tp/l2tp_ppp.c | 118 ++++++++++++++++++++++++--------------------
    1 file changed, 64 insertions(+), 54 deletions(-)

    diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
    index db2e584c625e..68d02e217ca3 100644
    --- a/net/l2tp/l2tp_ppp.c
    +++ b/net/l2tp/l2tp_ppp.c
    @@ -650,6 +650,65 @@ static int pppol2tp_tunnel_mtu(const struct l2tp_tunnel *tunnel)
    return mtu - PPPOL2TP_HEADER_OVERHEAD;
    }

    +static struct l2tp_tunnel *pppol2tp_tunnel_get(struct net *net,
    + struct l2tp_connect_info *info,
    + bool *new_tunnel)
    +{
    + struct l2tp_tunnel *tunnel;
    + int error;
    +
    + *new_tunnel = false;
    +
    + tunnel = l2tp_tunnel_get(net, info->tunnel_id);
    +
    + /* Special case: create tunnel context if session_id and
    + * peer_session_id is 0. Otherwise look up tunnel using supplied
    + * tunnel id.
    + */
    + if (!info->session_id && !info->peer_session_id) {
    + if (!tunnel) {
    + struct l2tp_tunnel_cfg tcfg = {
    + .encap = L2TP_ENCAPTYPE_UDP,
    + };
    +
    + /* Prevent l2tp_tunnel_register() from trying to set up
    + * a kernel socket.
    + */
    + if (info->fd < 0)
    + return ERR_PTR(-EBADF);
    +
    + error = l2tp_tunnel_create(info->fd,
    + info->version,
    + info->tunnel_id,
    + info->peer_tunnel_id, &tcfg,
    + &tunnel);
    + if (error < 0)
    + return ERR_PTR(error);
    +
    + l2tp_tunnel_inc_refcount(tunnel);
    + error = l2tp_tunnel_register(tunnel, net, &tcfg);
    + if (error < 0) {
    + kfree(tunnel);
    + return ERR_PTR(error);
    + }
    +
    + *new_tunnel = true;
    + }
    + } else {
    + /* Error if we can't find the tunnel */
    + if (!tunnel)
    + return ERR_PTR(-ENOENT);
    +
    + /* Error if socket is not prepped */
    + if (!tunnel->sock) {
    + l2tp_tunnel_dec_refcount(tunnel);
    + return ERR_PTR(-ENOENT);
    + }
    + }
    +
    + return tunnel;
    +}
    +
    /* connect() handler. Attach a PPPoX socket to a tunnel UDP socket
    */
    static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
    @@ -663,7 +722,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
    struct pppol2tp_session *ps;
    struct l2tp_session_cfg cfg = { 0, };
    bool drop_refcnt = false;
    - bool drop_tunnel = false;
    bool new_session = false;
    bool new_tunnel = false;
    int error;
    @@ -672,6 +730,10 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
    if (error < 0)
    return error;

    + tunnel = pppol2tp_tunnel_get(sock_net(sk), &info, &new_tunnel);
    + if (IS_ERR(tunnel))
    + return PTR_ERR(tunnel);
    +
    lock_sock(sk);

    /* Check for already bound sockets */
    @@ -689,57 +751,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
    if (!info.tunnel_id)
    goto end;

    - tunnel = l2tp_tunnel_get(sock_net(sk), info.tunnel_id);
    - if (tunnel)
    - drop_tunnel = true;
    -
    - /* Special case: create tunnel context if session_id and
    - * peer_session_id is 0. Otherwise look up tunnel using supplied
    - * tunnel id.
    - */
    - if (!info.session_id && !info.peer_session_id) {
    - if (!tunnel) {
    - struct l2tp_tunnel_cfg tcfg = {
    - .encap = L2TP_ENCAPTYPE_UDP,
    - };
    -
    - /* Prevent l2tp_tunnel_register() from trying to set up
    - * a kernel socket.
    - */
    - if (info.fd < 0) {
    - error = -EBADF;
    - goto end;
    - }
    -
    - error = l2tp_tunnel_create(info.fd,
    - info.version,
    - info.tunnel_id,
    - info.peer_tunnel_id, &tcfg,
    - &tunnel);
    - if (error < 0)
    - goto end;
    -
    - l2tp_tunnel_inc_refcount(tunnel);
    - error = l2tp_tunnel_register(tunnel, sock_net(sk),
    - &tcfg);
    - if (error < 0) {
    - kfree(tunnel);
    - goto end;
    - }
    - drop_tunnel = true;
    - new_tunnel = true;
    - }
    - } else {
    - /* Error if we can't find the tunnel */
    - error = -ENOENT;
    - if (!tunnel)
    - goto end;
    -
    - /* Error if socket is not prepped */
    - if (!tunnel->sock)
    - goto end;
    - }
    -
    if (tunnel->peer_tunnel_id == 0)
    tunnel->peer_tunnel_id = info.peer_tunnel_id;

    @@ -840,8 +851,7 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
    }
    if (drop_refcnt)
    l2tp_session_dec_refcount(session);
    - if (drop_tunnel)
    - l2tp_tunnel_dec_refcount(tunnel);
    + l2tp_tunnel_dec_refcount(tunnel);
    release_sock(sk);

    return error;
    --
    2.39.0
    \
     
     \ /
      Last update: 2023-03-27 00:19    [W:2.854 / U:0.136 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site