From nobody Thu Sep 08 12:49:09 2022 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4MNf8B6MfJz4c2m3; Thu, 8 Sep 2022 12:49:14 +0000 (UTC) (envelope-from tijl@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (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 "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4MNf8B5qPSz49mq; Thu, 8 Sep 2022 12:49:14 +0000 (UTC) (envelope-from tijl@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1662641354; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CvbmBVWlsYJsaQnTvB8UfONLGreTq1QKrAaEU0R9lL8=; b=yhDb5WyPVHqQ9q4ExTilz2uL/72FiIKVSERcNkR0/xLfnFJwk0Bl4o1cuM4AnrdbRSlL0D aLxclFi9A+aaRLMs/dCq5RmJkwoDw6BRzRK55SmGdfW/Gf6V9YsoeEOy5tCEb8mFhKpven i7DpWREm+lGteAteuwTA3JqcKO+AI8p23mECT+G6qELYmmnKFhOcq9ioSCE/SbQqVjW1hk gZFfq1YZfeZYE1Geh9uG4BlpT5xFXxL6BlqP+mLgjO/N/udRmZxSvvND/QOoagPorYZdna NZsilTT/tqQLS8CLWj2u67Qrdnfz+Ff2WGehTvLPjkP7aseops0PwQ7qhRfaZw== Received: from localhost (unknown [IPv6:2a02:a03f:894b:4700:fd1e:4ba8:eecd:e544]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: tijl) by smtp.freebsd.org (Postfix) with ESMTPSA id 4MNf896mnfz18Gf; Thu, 8 Sep 2022 12:49:13 +0000 (UTC) (envelope-from tijl@FreeBSD.org) Date: Thu, 8 Sep 2022 14:49:09 +0200 From: =?UTF-8?B?VMSzbA==?= Coosemans To: Konstantin Belousov Cc: pho@freebsd.org, src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 95f773e59482 - main - i386 copyout_fast: improve detection of a fault on accessing userspace Message-ID: <20220908144909.7e6a0d6b@FreeBSD.org> In-Reply-To: References: <202208241925.27OJP9Fh069091@gitrepo.freebsd.org> <20220906171826.1629cfcf@FreeBSD.org> <20220906231745.1a0f3c15@FreeBSD.org> <20220907183804.29829b14@FreeBSD.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1662641354; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=CvbmBVWlsYJsaQnTvB8UfONLGreTq1QKrAaEU0R9lL8=; b=MbDVL64ET2/ku/FVWmNEyKRYuMTDp8v4RM9b0sPrMOdB1BS6IR9aQRm6MewSi0BxlbwPrH qhnckuKXqliwzFle1x6Fft6zR8caWFZZDFfVj77L4seKiVnyo+uEr1+Ta0fxS9zjYM8ZLQ 7hvkZStNCRjt1EYa5nuqJlBWtp55LLGT7JdnN3XodUP2zGZjCO6TvxSWqSwTx4JScpixb4 xC6vPwgs5JVqLM0p9g0KoyQVfubExD3k6DaOuPe1DXTbsWl1yKrgU788ZxMIgQlq3L5VbP mnI6v+tpSdXYQmsgaFCkekqGYNdy5rnKxfNUuU8JZpX4dlx3OzlcnDg6ePM0fw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1662641354; a=rsa-sha256; cv=none; b=GWKUj5/mZ3Oa5tKSs6acdyNEVlf/IxQycalkRW3aycoMlHvq/6DLnuqIS6k2wA4BKIMKR1 FyOY3ktsGeJOkzgsJ8O0gJJsUxL5N6IArSfgp6i0ZzZBQveg84iXa+cper9N7tmziPcYQ0 dneK6uObBtFNOSH5dh24pPamphhzgg8WXpWbwNaPutdyy5czSdmMbVe1zUBrMB5W5Rwyh/ G+XtTj4ZyhtG1pBkiG46QOE43nHO/pIBlPvvvB9j4o6PgakQCaxnE3GqttCcOeBy+3jatS Fpdq9r85n7ZZFXpNPPfZbpVMUrLxmg03G8MYsUL9CmmhB9SB4yF0IAUz5AuH1A== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N On Wed, 7 Sep 2022 23:18:32 +0300 Konstantin Belousov wrote: > On Wed, Sep 07, 2022 at 06:38:04PM +0200, T=C4=B3l Coosemans wrote: >> So interrupts must have been reenabled somehow, probably by the page >> fault handler, and this allows context switches and then another process >> can call copyout or copyin and corrupt the trampoline stack and >> copyout_buf. =20 > I do not see where the interrupts could be reenabled in copyout_fast path, > without or with page fault on the userspace access. The problem is not with userspace page faults. Those are treated specially by the page fault handler in exception.s causing copyout_fast and copyin_fast to return immediately with EFAULT so copyout and copyin fall back to doing a slow copy. The problem is with page faults on the kernel space accesses. Before this commit they were also treated specially, and now they are not. Now=20 the page fault handler in exception.s calls trap() which calls trap_pfault() etc. > Also I am not sure what should your patch demonstrate. My patch adds a trylock after cli and an unlock before sti. If the trylock fails panic("") is called. I put the lock at the base of the trampoline stack so it's per cpu. The code between cli and sti is supposed to be uninterruptible so the trylock should never fail, but it does! Somehow there's a context switch to another thread that calls copyout or copyin while the first thread was still busy copying when it was switched out. This corrupts the trampoline stack and/or copyout_buf of the first thread. If this isn't caused by interrupts being reenabled then the context switch must be happening in some other way. And this must be happening because page faults on kernel space accesses call trap() now. > Also, pmap_trm_alloc() puts guard page between consequent allocations, > so trampoline stack overflow must be very careful to not really overflow, > but just touch enough bytes to give the effect. I no longer think stack overflow happens. I tried increasing the trampoline stack to 4 pages (=3DKSTACK_PAGES) and the panic still looked the same, only with stack addresses shifted by 3 pages. > But lets check the hypothesis. If interrupts are enabled somehow, then > processor would execute interrupt/fault handler on the same stack, which > is trampoline stack and not the kstack. I added an INVARIANTS check that > verifies that both trap() and syscall() use kstack. Could you, please, > fetch my ast branch and see what it the outcome? I have not done this yet, but I don't think this will ever trigger. For copyin_fast, interrupts are guaranteed to be disabled for copying from user to copyout_buf. In your ast branch subsequent copying from copyout_buf to kernel space runs on the kstack so any interrupts here won't use the trampoline stack. For copyout_fast, page faults when copying from the kernel to copyout_buf must be extremely rare because the kernel just prepared the data to be copied. And without page faults interrupts stay disabled for subsequent copying from copyout_buf to user space. I've never seen the problem with copyout_fast, only with copyin_fast.