netdev - Re: PATCH ax25: Don't hold skb lock while doing blocking read

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

Date:   Sun, 26 Jul 2020 04:28:59 -0400
From:   thomas@...ets.se
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     netdev@...r.kernel.org, Ralf Baechle <ralf@...ux-mips.org>
Subject: Re: PATCH ax25: Don't hold skb lock while doing blocking read

On Mon, 13 Jul 2020 22:31:18 +0100, Eric Dumazet <eric.dumazet@...il.com> said:
> Or you could use a smaller change and make this look like net/x25/af_x25.c ?

Ah, good point.
Here's a revised version, that like x25 releases the lock and reacquires it.


Author: Thomas Habets <habets@...gle.com>
Date:   Fri Jun 26 15:23:26 2020 +0100

    ax25: Don't hold sock lock while doing blocking read

    This release/lock follows the pattern of net/x25/af_x25.c.

    I see some other socket types are also locking during
    read. E.g. qrtr_recvmsg. Maybe they need to be fixed too.

    Here's a test program that illustrates the problem:
    https://github.com/ThomasHabets/radiostuff/blob/master/ax25/axftp/examples/client_lockcheck.cc

    Before:
    strace -f -eread,write ./examples/client_lockcheck M0THC-9 M0THC-0 M0THC-2
    strace: Process 3888 attached
    [pid  3888] read(3,  <unfinished ...>
    [pid  3887] write(3, "hello world", 11
    [hang]

    After:
    strace -f -eread,write ./examples/client_lockcheck M0THC-9 M0THC-0 M0THC-2
    strace: Process 2433 attached
    [pid  2433] read(3,  <unfinished ...>
    [pid  2432] write(3, "hello world", 11) = 11
    [pid  2433] <... read resumed> "yo", 1000) = 2
    [pid  2433] write(1, "yo\n", 3yo
    )         = 3
    [successful exit]

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index fd91cd34f25e..75a7c32c7c1a 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -1628,8 +1628,10 @@ static int ax25_recvmsg(struct socket *sock,
struct msghdr *msg, size_t size,
        }

        /* Now we can treat all alike */
+       release_sock(sk);
        skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
                                flags & MSG_DONTWAIT, &err);
+       lock_sock(sk);
        if (skb == NULL)
                goto out;