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=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, 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 82DFEC433FE for ; Mon, 7 Dec 2020 12:02:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2BDC323370 for ; Mon, 7 Dec 2020 12:02:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726980AbgLGMCF (ORCPT ); Mon, 7 Dec 2020 07:02:05 -0500 Received: from foss.arm.com ([217.140.110.172]:48686 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726370AbgLGMCE (ORCPT ); Mon, 7 Dec 2020 07:02:04 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 02E561042; Mon, 7 Dec 2020 04:01:19 -0800 (PST) Received: from [10.57.61.6] (unknown [10.57.61.6]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7C97D3F718; Mon, 7 Dec 2020 04:01:16 -0800 (PST) Subject: Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map To: Keqian Zhu , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, iommu@lists.linux-foundation.org Cc: Will Deacon , Marc Zyngier , Joerg Roedel , Catalin Marinas , James Morse , Suzuki K Poulose , Sean Christopherson , Julien Thierry , Mark Brown , Thomas Gleixner , Andrew Morton , Alexios Zavras , wanghaibin.wang@huawei.com, jiangkunkun@huawei.com References: <20201205082957.12544-1-zhukeqian1@huawei.com> From: Robin Murphy Message-ID: Date: Mon, 7 Dec 2020 12:01:09 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: <20201205082957.12544-1-zhukeqian1@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2020-12-05 08:29, Keqian Zhu wrote: > ... then we have more chance to detect wrong code logic. I don't follow that justification - it's still the same check with the same outcome, so how does moving it have any effect on the chance to detect errors? AFAICS the only difference it would make is to make some errors *less* obvious - if a sufficiently broken caller passes an empty prot value alongside an invalid size or already-mapped address, this will now quietly hide the warnings from the more serious condition(s). Yes, it will bail out a bit faster in the specific case where the prot value is the only thing wrong, but since when do we optimise for fundamentally incorrect API usage? Robin. > Signed-off-by: Keqian Zhu > --- > drivers/iommu/io-pgtable-arm.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c > index a7a9bc08dcd1..8ade72adab31 100644 > --- a/drivers/iommu/io-pgtable-arm.c > +++ b/drivers/iommu/io-pgtable-arm.c > @@ -444,10 +444,6 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, > arm_lpae_iopte prot; > long iaext = (s64)iova >> cfg->ias; > > - /* If no access, then nothing to do */ > - if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE))) > - return 0; > - > if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size)) > return -EINVAL; > > @@ -456,6 +452,10 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova, > if (WARN_ON(iaext || paddr >> cfg->oas)) > return -ERANGE; > > + /* If no access, then nothing to do */ > + if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE))) > + return 0; > + > prot = arm_lpae_prot_to_pte(data, iommu_prot); > ret = __arm_lpae_map(data, iova, paddr, size, prot, lvl, ptep, gfp); > /* >