From owner-svn-src-all@freebsd.org Tue Nov 17 03:34:02 2020 Return-Path: Delivered-To: svn-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 5CB8B47E7E3; Tue, 17 Nov 2020 03:34:02 +0000 (UTC) (envelope-from kevans@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 "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4CZs4B29Tkz3LBl; Tue, 17 Nov 2020 03:34:02 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 3DCA5198A9; Tue, 17 Nov 2020 03:34:02 +0000 (UTC) (envelope-from kevans@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id 0AH3Y2YU006279; Tue, 17 Nov 2020 03:34:02 GMT (envelope-from kevans@FreeBSD.org) Received: (from kevans@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id 0AH3Y1Ya006276; Tue, 17 Nov 2020 03:34:01 GMT (envelope-from kevans@FreeBSD.org) Message-Id: <202011170334.0AH3Y1Ya006276@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: kevans set sender to kevans@FreeBSD.org using -f From: Kyle Evans Date: Tue, 17 Nov 2020 03:34:01 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r367743 - in head: lib/libthr/tests sys/kern X-SVN-Group: head X-SVN-Commit-Author: kevans X-SVN-Commit-Paths: in head: lib/libthr/tests sys/kern X-SVN-Commit-Revision: 367743 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Nov 2020 03:34:02 -0000 Author: kevans Date: Tue Nov 17 03:34:01 2020 New Revision: 367743 URL: https://svnweb.freebsd.org/changeset/base/367743 Log: _umtx_op: fix a compat32 bug in UMTX_OP_NWAKE_PRIVATE Specifically, if we're waking up some value n > BATCH_SIZE, then the copyin(9) is wrong on the second iteration due to upp being the wrong type. upp is currently a uint32_t**, so upp + pos advances it by twice as many elements as it should (host pointer size vs. compat32 pointer size). Fix it by just making upp a uint32_t*; it's still technically a double pointer, but the distinction doesn't matter all that much here since we're just doing arithmetic on it. Add a test case that demonstrates the problem, placed with the libthr tests since one messing with _umtx_op should be running these tests. Running under compat32, the new test case will hang as threads after the first 128 get missed in the wake. it's not immediately clear how to hit it in practice, since pthread_cond_broadcast() uses a smaller (sleepq batch?) size observed to be around ~50 -- I did not spend much time digging into it. The uintptr_t change makes no functional difference, but i've tossed it in since it's more accurate (semantically). Reported by: Andrew Gierth (andrew_tao173.riddles.org.uk, inspection) Reviewed by: kib MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D27231 Added: head/lib/libthr/tests/umtx_op_test.c (contents, props changed) Modified: head/lib/libthr/tests/Makefile head/sys/kern/kern_umtx.c Modified: head/lib/libthr/tests/Makefile ============================================================================== --- head/lib/libthr/tests/Makefile Tue Nov 17 03:26:56 2020 (r367742) +++ head/lib/libthr/tests/Makefile Tue Nov 17 03:34:01 2020 (r367743) @@ -35,6 +35,8 @@ NETBSD_ATF_TESTS_SH+= cancel_test NETBSD_ATF_TESTS_SH+= exit_test NETBSD_ATF_TESTS_SH+= resolv_test +ATF_TESTS_C+= umtx_op_test + LIBADD+= pthread LIBADD.fpu_test+= m LIBADD.sem_test+= rt Added: head/lib/libthr/tests/umtx_op_test.c ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/lib/libthr/tests/umtx_op_test.c Tue Nov 17 03:34:01 2020 (r367743) @@ -0,0 +1,108 @@ +/*- + * SPDX-License-Identifier: BSD-2-Clause + * + * Copyright (c) 2020 Kyle Evans + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include +__FBSDID("$FreeBSD$"); + +#include +#include + +#include + +#include + +/* + * This is an implementation detail of _umtx_op(2), pulled from + * sys/kern/kern_umtx.c. The relevant bug observed that requests above the + * batch side would not function as intended, so it's important that this + * reflects the BATCH_SIZE configured there. + */ +#define UMTX_OP_BATCH_SIZE 128 +#define THREAD_COUNT ((UMTX_OP_BATCH_SIZE * 3) / 2) + +static pthread_mutex_t static_mutex = PTHREAD_MUTEX_INITIALIZER; + +static int batched_waiting; + +static void * +batching_threadfunc(void *arg) +{ + + pthread_mutex_lock(&static_mutex); + ++batched_waiting; + pthread_mutex_unlock(&static_mutex); + _umtx_op(arg, UMTX_OP_WAIT_UINT_PRIVATE, 0, NULL, NULL); + + return (NULL); +} + +ATF_TC(batching); +ATF_TC_HEAD(batching, tc) +{ + atf_tc_set_md_var(tc, "descr", + "Checks batching of UMTX_OP_NWAKE_PRIVATE"); +} +ATF_TC_BODY(batching, tc) +{ + uintptr_t addrs[THREAD_COUNT]; + uint32_t vals[THREAD_COUNT]; + pthread_t threads[THREAD_COUNT]; + + for (int i = 0; i < THREAD_COUNT; i++) { + addrs[i] = (uintptr_t)&vals[i]; + vals[i] = 0; + pthread_create(&threads[i], NULL, batching_threadfunc, + &vals[i]); + } + + pthread_mutex_lock(&static_mutex); + while (batched_waiting != THREAD_COUNT) { + pthread_mutex_unlock(&static_mutex); + pthread_yield(); + pthread_mutex_lock(&static_mutex); + } + + /* + * Spin for another .50 seconds to make sure they're all safely in the + * kernel. + */ + usleep(500000); + + pthread_mutex_unlock(&static_mutex); + _umtx_op(addrs, UMTX_OP_NWAKE_PRIVATE, THREAD_COUNT, NULL, NULL); + + for (int i = 0; i < THREAD_COUNT; i++) { + ATF_REQUIRE_EQ(0, pthread_join(threads[i], NULL)); + } +} + +ATF_TP_ADD_TCS(tp) +{ + + ATF_TP_ADD_TC(tp, batching); + return (atf_no_error()); +} Modified: head/sys/kern/kern_umtx.c ============================================================================== --- head/sys/kern/kern_umtx.c Tue Nov 17 03:26:56 2020 (r367742) +++ head/sys/kern/kern_umtx.c Tue Nov 17 03:34:01 2020 (r367743) @@ -4370,10 +4370,10 @@ __umtx_op_sem2_wait_compat32(struct thread *td, struct static int __umtx_op_nwake_private32(struct thread *td, struct _umtx_op_args *uap) { - uint32_t uaddrs[BATCH_SIZE], **upp; + uint32_t uaddrs[BATCH_SIZE], *upp; int count, error, i, pos, tocopy; - upp = (uint32_t **)uap->obj; + upp = (uint32_t *)uap->obj; error = 0; for (count = uap->val, pos = 0; count > 0; count -= tocopy, pos += tocopy) { @@ -4382,7 +4382,7 @@ __umtx_op_nwake_private32(struct thread *td, struct _u if (error != 0) break; for (i = 0; i < tocopy; ++i) - kern_umtx_wake(td, (void *)(intptr_t)uaddrs[i], + kern_umtx_wake(td, (void *)(uintptr_t)uaddrs[i], INT_MAX, 1); maybe_yield(); }