From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C798FC07E95 for ; Wed, 7 Jul 2021 12:50:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A980B61CBE for ; Wed, 7 Jul 2021 12:50:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231775AbhGGMwt (ORCPT ); Wed, 7 Jul 2021 08:52:49 -0400 Received: from mail.kernel.org ([198.145.29.99]:51074 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231383AbhGGMws (ORCPT ); Wed, 7 Jul 2021 08:52:48 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 887AC61CBA; Wed, 7 Jul 2021 12:50:06 +0000 (UTC) Date: Wed, 7 Jul 2021 13:50:03 +0100 From: Catalin Marinas To: Robin Murphy Cc: Chen Huang , Al Viro , Matthew Wilcox , Christoph Hellwig , Mark Rutland , Andrew Morton , Stephen Rothwell , Randy Dunlap , Will Deacon , Linux ARM , linux-mm , open list Subject: Re: [BUG] arm64: an infinite loop in generic_perform_write() Message-ID: <20210707125003.GA24397@arm.com> References: <27fbb8c1-2a65-738f-6bec-13f450395ab7@arm.com> <20210624185554.GC25097@arm.com> <20210625103905.GA20835@arm.com> <7f14271a-9b2f-1afc-3caf-c4e5b36efa73@arm.com> <20210706175052.GD15218@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 06, 2021 at 08:15:47PM +0100, Robin Murphy wrote: > On 2021-07-06 18:50, Catalin Marinas wrote: > > On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote: > > > @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user) > > > .section .fixup,"ax" > > > .align 2 > > > +9997: cmp dst, dstin > > > + b.ne 9998f > > > + // Before being absolutely sure we couldn't copy anything, try harder > > > + ldrb tmp1w, [srcin] > > > +USER(9998f, sttrb tmp1w, [dstin]) > > > + add dst, dstin, #1 > > > 9998: sub x0, end, dst // bytes not copied > > > ret > > > .previous > > > > I think it's worth doing the copy_to_user() fallback in a loop until it > > faults or hits the end of the buffer. This would solve the problem we > > currently have with writing more bytes than actually reported. The > > copy_from_user() is not necessary, a byte would suffice. > > The thing is, we don't really have that problem since the set_fs cleanup > removed IMP-DEF STP behaviour from the picture - even with the current mess > we could perfectly well know which of the two STTRs faulted if we just put a > little more effort in. I think there are some corner cases: STTR across a page boundary, faulting on the second page. The architecture allows some data to be written (or not) in the first page, so we'd under-report if we use the destination update. If we use the fault address it's even worse as we may over-report in case the instruction did not write anything. > But yuck... If you think the potential under-reporting is worth fixing right > now, rather than just letting it disappear in a future rewrite, then I'd > still rather do it by passing the actual fault address to the current > copy_to_user fixup. After some more digging in the ARM ARM, I don't think that's fixable by using the actual fault address. B2.2.1 and D1.13.5 in version G.a (thanks to Will for digging them out) mean that for an interrupted store (exception, interrupt), any bytes stored by the instruction become UNKNOWN. In practice, this means left unchanged or written. So I think a byte-wise write loop is the only chance we have at a more precise reporting, unless we change the loops to align the writes. > A retry loop could still technically under-report if the > page disappears (or tag changes) between faulting on the second word of a > pair and retrying from the first, so we'd want to pin the initial fault down > to a single access anyway. All the loop would achieve after that is > potentially fill in an extra 1-7 bytes right up to the offending page/tag > boundary for the sake of being nice, which I remain unconvinced is worth the > bother :) There is indeed the risk of a race but we can blame the user for concurrently changing the permissions or tag. The kernel wouldn't normally do this. -- Catalin From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A6802C07E9B for ; Wed, 7 Jul 2021 12:51:34 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7926361CB9 for ; Wed, 7 Jul 2021 12:51:34 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7926361CB9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=R80HDXD/Akj9jnDYBOCYaGoTRv/HoP+YbnjMB5oizBw=; b=SMwRslzXyfYvLF jyN/2094MCGYC8JF94J4Cf/I8denWlaFh2kjWf9TRbJ1Tu5VJ8gr6FhBH0v4oeT5Tzu4t7jqojc+Q ixPl7lDKsI1J1j+xMIvQvREtBI326ME+Pmvuiro2Fn3BV3OG/mr4nbNelJbdj7cqAigUuajCip1Hc 4EUWrMOJ76HafwyxFeEZZbbDoW1pyBWXLPGCqJR9bypP4w6FUsXz03MNEe/9SMi3ahaGCz+P2bHmB XqFKvij1n+cgI6CEbVcmUhUDu6RaRp7jdk5lB0blNfhqOXv1X/tzjVpxQ7PqA4/smBc15vIX2/5gR g7w9D/XqSbYcugBlHC4g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m170H-00EoXg-Sl; Wed, 07 Jul 2021 12:50:14 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1m170C-00EoWQ-Us for linux-arm-kernel@lists.infradead.org; Wed, 07 Jul 2021 12:50:10 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 887AC61CBA; Wed, 7 Jul 2021 12:50:06 +0000 (UTC) Date: Wed, 7 Jul 2021 13:50:03 +0100 From: Catalin Marinas To: Robin Murphy Cc: Chen Huang , Al Viro , Matthew Wilcox , Christoph Hellwig , Mark Rutland , Andrew Morton , Stephen Rothwell , Randy Dunlap , Will Deacon , Linux ARM , linux-mm , open list Subject: Re: [BUG] arm64: an infinite loop in generic_perform_write() Message-ID: <20210707125003.GA24397@arm.com> References: <27fbb8c1-2a65-738f-6bec-13f450395ab7@arm.com> <20210624185554.GC25097@arm.com> <20210625103905.GA20835@arm.com> <7f14271a-9b2f-1afc-3caf-c4e5b36efa73@arm.com> <20210706175052.GD15218@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210707_055009_091312_65F654AC X-CRM114-Status: GOOD ( 26.82 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jul 06, 2021 at 08:15:47PM +0100, Robin Murphy wrote: > On 2021-07-06 18:50, Catalin Marinas wrote: > > On Mon, Jun 28, 2021 at 05:22:30PM +0100, Robin Murphy wrote: > > > @@ -62,6 +64,12 @@ EXPORT_SYMBOL(__arch_copy_to_user) > > > .section .fixup,"ax" > > > .align 2 > > > +9997: cmp dst, dstin > > > + b.ne 9998f > > > + // Before being absolutely sure we couldn't copy anything, try harder > > > + ldrb tmp1w, [srcin] > > > +USER(9998f, sttrb tmp1w, [dstin]) > > > + add dst, dstin, #1 > > > 9998: sub x0, end, dst // bytes not copied > > > ret > > > .previous > > > > I think it's worth doing the copy_to_user() fallback in a loop until it > > faults or hits the end of the buffer. This would solve the problem we > > currently have with writing more bytes than actually reported. The > > copy_from_user() is not necessary, a byte would suffice. > > The thing is, we don't really have that problem since the set_fs cleanup > removed IMP-DEF STP behaviour from the picture - even with the current mess > we could perfectly well know which of the two STTRs faulted if we just put a > little more effort in. I think there are some corner cases: STTR across a page boundary, faulting on the second page. The architecture allows some data to be written (or not) in the first page, so we'd under-report if we use the destination update. If we use the fault address it's even worse as we may over-report in case the instruction did not write anything. > But yuck... If you think the potential under-reporting is worth fixing right > now, rather than just letting it disappear in a future rewrite, then I'd > still rather do it by passing the actual fault address to the current > copy_to_user fixup. After some more digging in the ARM ARM, I don't think that's fixable by using the actual fault address. B2.2.1 and D1.13.5 in version G.a (thanks to Will for digging them out) mean that for an interrupted store (exception, interrupt), any bytes stored by the instruction become UNKNOWN. In practice, this means left unchanged or written. So I think a byte-wise write loop is the only chance we have at a more precise reporting, unless we change the loops to align the writes. > A retry loop could still technically under-report if the > page disappears (or tag changes) between faulting on the second word of a > pair and retrying from the first, so we'd want to pin the initial fault down > to a single access anyway. All the loop would achieve after that is > potentially fill in an extra 1-7 bytes right up to the offending page/tag > boundary for the sake of being nice, which I remain unconvinced is worth the > bother :) There is indeed the risk of a race but we can blame the user for concurrently changing the permissions or tag. The kernel wouldn't normally do this. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel