From owner-dev-commits-src-all@freebsd.org Fri Sep 10 21:27:59 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id A20E1664FD9; Fri, 10 Sep 2021 21:27:59 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4H5prH3Gckz4k2S; Fri, 10 Sep 2021 21:27:59 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4FA5D1468D; Fri, 10 Sep 2021 21:27:59 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 18ALRxPt020690; Fri, 10 Sep 2021 21:27:59 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 18ALRx1g020689; Fri, 10 Sep 2021 21:27:59 GMT (envelope-from git) Date: Fri, 10 Sep 2021 21:27:59 GMT Message-Id: <202109102127.18ALRx1g020689@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: 141fe2dceeae - main - aio: Interlock with listen(2) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 141fe2dceeaeefaaffc2242c8652345a081e825a Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Sep 2021 21:27:59 -0000 The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=141fe2dceeaeefaaffc2242c8652345a081e825a commit 141fe2dceeaeefaaffc2242c8652345a081e825a Author: Mark Johnston AuthorDate: 2021-09-10 21:21:11 +0000 Commit: Mark Johnston CommitDate: 2021-09-10 21:21:11 +0000 aio: Interlock with listen(2) soo_aio_queue() did not handle the possibility that the provided socket is a listening socket. Up until recently, to fix this one would have to acquire the socket lock first and check, since the socket buffer locks were destroyed by listen(2). Now that the socket buffer locks belong to the socket, simply check SOLISTENING(so) after acquiring them, and make listen(2) return an error if any AIO jobs are enqueued on the socket. Add a couple of simple regression test cases. Note that this fixes things only for the default AIO implementation; cxgbe(4)'s TCP offload has a separate pru_aio_queue implementation which requires its own solution. Reported by: syzbot+c8aa122fa2c6a4e2a28b@syzkaller.appspotmail.com Reported by: syzbot+39af117d43d4f0faf512@syzkaller.appspotmail.com Reported by: syzbot+60cceb9569145a0b993b@syzkaller.appspotmail.com Reported by: syzbot+2d522c5db87710277ca5@syzkaller.appspotmail.com Reviewed by: tuexen, gallatin, jhb Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D31901 --- sys/kern/sys_socket.c | 12 +++++++- sys/kern/uipc_socket.c | 7 +++++ tests/sys/aio/aio_test.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 95 insertions(+), 2 deletions(-) diff --git a/sys/kern/sys_socket.c b/sys/kern/sys_socket.c index e53b0367960b..83dc1cb2622b 100644 --- a/sys/kern/sys_socket.c +++ b/sys/kern/sys_socket.c @@ -808,18 +808,28 @@ soo_aio_queue(struct file *fp, struct kaiocb *job) if (error == 0) return (0); + /* Lock through the socket, since this may be a listening socket. */ switch (job->uaiocb.aio_lio_opcode & (LIO_WRITE | LIO_READ)) { case LIO_READ: sb = &so->so_rcv; + SOCK_RECVBUF_LOCK(so); break; case LIO_WRITE: sb = &so->so_snd; + SOCK_SENDBUF_LOCK(so); break; default: return (EINVAL); } - SOCKBUF_LOCK(sb); + if (SOLISTENING(so)) { + if (sb == &so->so_rcv) + SOCK_RECVBUF_UNLOCK(so); + else + SOCK_SENDBUF_UNLOCK(so); + return (EINVAL); + } + if (!aio_set_cancel_function(job, soo_aio_cancel)) panic("new job was cancelled"); TAILQ_INSERT_TAIL(&sb->sb_aiojobq, job, list); diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c index a502b06ce00e..cbddd80e0546 100644 --- a/sys/kern/uipc_socket.c +++ b/sys/kern/uipc_socket.c @@ -928,6 +928,13 @@ solisten_proto_check(struct socket *so) } mtx_lock(&so->so_snd_mtx); mtx_lock(&so->so_rcv_mtx); + + /* Interlock with soo_aio_queue(). */ + if ((so->so_snd.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0 || + (so->so_rcv.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0) { + solisten_proto_abort(so); + return (EINVAL); + } return (0); } diff --git a/tests/sys/aio/aio_test.c b/tests/sys/aio/aio_test.c index 1c694ad0c18b..a81cb906e38a 100644 --- a/tests/sys/aio/aio_test.c +++ b/tests/sys/aio/aio_test.c @@ -40,11 +40,12 @@ */ #include +#include #include #include #include #include -#include +#include #include #include @@ -1177,6 +1178,79 @@ ATF_TC_BODY(aio_socket_blocking_short_write_vectored, tc) aio_socket_blocking_short_write_test(true); } +/* + * Verify that AIO requests fail when applied to a listening socket. + */ +ATF_TC_WITHOUT_HEAD(aio_socket_listen_fail); +ATF_TC_BODY(aio_socket_listen_fail, tc) +{ + struct aiocb iocb; + struct sockaddr_un sun; + char buf[16]; + int s; + + s = socket(AF_LOCAL, SOCK_STREAM, 0); + ATF_REQUIRE(s != -1); + + memset(&sun, 0, sizeof(sun)); + snprintf(sun.sun_path, sizeof(sun.sun_path), "%s", "listen.XXXXXX"); + mktemp(sun.sun_path); + sun.sun_family = AF_LOCAL; + sun.sun_len = SUN_LEN(&sun); + + ATF_REQUIRE(bind(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) == 0); + ATF_REQUIRE(listen(s, 5) == 0); + + memset(buf, 0, sizeof(buf)); + memset(&iocb, 0, sizeof(iocb)); + iocb.aio_fildes = s; + iocb.aio_buf = buf; + iocb.aio_nbytes = sizeof(buf); + + ATF_REQUIRE_ERRNO(EINVAL, aio_read(&iocb) == -1); + ATF_REQUIRE_ERRNO(EINVAL, aio_write(&iocb) == -1); + + ATF_REQUIRE(unlink(sun.sun_path) == 0); + close(s); +} + +/* + * Verify that listen(2) fails if a socket has pending AIO requests. + */ +ATF_TC_WITHOUT_HEAD(aio_socket_listen_pending); +ATF_TC_BODY(aio_socket_listen_pending, tc) +{ + struct aiocb iocb; + struct sockaddr_un sun; + char buf[16]; + int s; + + s = socket(AF_LOCAL, SOCK_STREAM, 0); + ATF_REQUIRE(s != -1); + + memset(&sun, 0, sizeof(sun)); + snprintf(sun.sun_path, sizeof(sun.sun_path), "%s", "listen.XXXXXX"); + mktemp(sun.sun_path); + sun.sun_family = AF_LOCAL; + sun.sun_len = SUN_LEN(&sun); + + ATF_REQUIRE(bind(s, (struct sockaddr *)&sun, SUN_LEN(&sun)) == 0); + + memset(buf, 0, sizeof(buf)); + memset(&iocb, 0, sizeof(iocb)); + iocb.aio_fildes = s; + iocb.aio_buf = buf; + iocb.aio_nbytes = sizeof(buf); + ATF_REQUIRE(aio_read(&iocb) == 0); + + ATF_REQUIRE_ERRNO(EINVAL, listen(s, 5) == -1); + + ATF_REQUIRE(aio_cancel(s, &iocb) != -1); + + ATF_REQUIRE(unlink(sun.sun_path) == 0); + close(s); +} + /* * This test verifies that cancelling a partially completed socket write * returns a short write rather than ECANCELED. @@ -1808,6 +1882,8 @@ ATF_TP_ADD_TCS(tp) ATF_TP_ADD_TC(tp, aio_socket_two_reads); ATF_TP_ADD_TC(tp, aio_socket_blocking_short_write); ATF_TP_ADD_TC(tp, aio_socket_blocking_short_write_vectored); + ATF_TP_ADD_TC(tp, aio_socket_listen_fail); + ATF_TP_ADD_TC(tp, aio_socket_listen_pending); ATF_TP_ADD_TC(tp, aio_socket_short_write_cancel); ATF_TP_ADD_TC(tp, aio_writev_dos_iov_len); ATF_TP_ADD_TC(tp, aio_writev_dos_iovcnt);