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.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED 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 B5C4CC433E0 for ; Thu, 25 Feb 2021 18:01:00 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 553EC64F44 for ; Thu, 25 Feb 2021 18:01:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 553EC64F44 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id DCD4F6B0006; Thu, 25 Feb 2021 13:00:59 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D7E308D0002; Thu, 25 Feb 2021 13:00:59 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C6D6D8D0001; Thu, 25 Feb 2021 13:00:59 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0109.hostedemail.com [216.40.44.109]) by kanga.kvack.org (Postfix) with ESMTP id B16E56B0006 for ; Thu, 25 Feb 2021 13:00:59 -0500 (EST) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 68966183BA091 for ; Thu, 25 Feb 2021 18:00:59 +0000 (UTC) X-FDA: 77857556238.13.E252B19 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf30.hostedemail.com (Postfix) with ESMTP id C7D71E180B96 for ; Thu, 25 Feb 2021 18:00:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614276058; 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=Y4EVqTmbFoC++JsVxh4mL3ZlSM7yY9VUZbTEgL3/THo=; b=RU1e98Y7wtMcth613jJ7DE74fwzSlS24yYkVHuCA/PQ3XvKgK/Aafv+bZb38vDNzJZMR0C dtJNHbIlc0kWmuK6Iub2u3ba4Y1dpa2wY6BMmn2RFo8QcILfyPS1nlVOlJrw44eicO2YDA FdecJB0kzhtW2IESEWGmshYanTpEMDY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-86-SQt7MoggPxCY09yAxdWYew-1; Thu, 25 Feb 2021 13:00:52 -0500 X-MC-Unique: SQt7MoggPxCY09yAxdWYew-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4939B107ACE8; Thu, 25 Feb 2021 18:00:51 +0000 (UTC) Received: from omen.home.shazbot.org (ovpn-112-255.phx2.redhat.com [10.3.112.255]) by smtp.corp.redhat.com (Postfix) with ESMTP id C01BB5C237; Thu, 25 Feb 2021 18:00:50 +0000 (UTC) Date: Thu, 25 Feb 2021 11:00:50 -0700 From: Alex Williamson To: Steven Sistare Cc: Dan Carpenter , kbuild@lists.01.org, lkp@intel.com, kbuild-all@lists.01.org, Linux Memory Management List , Cornelia Huck Subject: Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' Message-ID: <20210225110050.5460c1c0@omen.home.shazbot.org> In-Reply-To: <7846d18a-36bd-6cfd-798c-3d6dc7ee1457@oracle.com> References: <20210222141043.GW2222@kadam> <20210222155145.50e2d513@omen.home.shazbot.org> <20210222161753.7acc4e92@omen.home.shazbot.org> <20210223104535.17986dee@omen.home.shazbot.org> <6527a7db-3b13-2572-3450-157e7de598c0@oracle.com> <20210223141001.765ae37f@omen.home.shazbot.org> <57e47f93-e8f2-fdc6-ad26-dfb6bdbe3a25@oracle.com> <9b1b847d-502d-2d6d-6610-a43ff5c7ba26@oracle.com> <20210224155524.7371d438@omen.home.shazbot.org> <7846d18a-36bd-6cfd-798c-3d6dc7ee1457@oracle.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=alex.williamson@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Stat-Signature: g9i3pcbgnsu9sbrsnnmrs5wais8deffa X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: C7D71E180B96 Received-SPF: none (redhat.com>: No applicable sender policy available) receiver=imf30; identity=mailfrom; envelope-from=""; helo=us-smtp-delivery-124.mimecast.com; client-ip=170.10.133.124 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1614276058-827799 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, 25 Feb 2021 10:25:16 -0500 Steven Sistare wrote: > On 2/24/2021 5:55 PM, Alex Williamson wrote: > > On Tue, 23 Feb 2021 18:58:18 -0500 > > Steven Sistare wrote: > > > >> On 2/23/2021 4:52 PM, Steven Sistare wrote: > >>> On 2/23/2021 4:10 PM, Alex Williamson wrote: > >>>> On Tue, 23 Feb 2021 15:37:31 -0500 > >>>> Steven Sistare wrote: > >>>> > >>>>> On 2/23/2021 12:45 PM, Alex Williamson wrote: > >>>>>> On Tue, 23 Feb 2021 08:56:36 -0500 > >>>>>> Steven Sistare wrote: > >>>>>> > >>>>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: > >>>>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 > >>>>>>>> Alex Williamson wrote: > >>>>>>>> > >>>>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 > >>>>>>>>> Dan Carpenter wrote: > >>>>>>>>> > >>>>>>>>>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > >>>>>>>>>> head: 37dfbfbdca66834bc0f64ec9b35e09ac6c8898da > >>>>>>>>>> commit: 0f53afa12baec8c00f5d1d6afb49325ada105253 [6931/12022] vfio/type1: unmap cleanup > >>>>>>>>> > >>>>>>>>> It's always the patches that claim no functional change... ;) > >>>>>>>>> > >>>>>>>>>> config: i386-randconfig-m021-20210222 (attached as .config) > >>>>>>>>>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > >>>>>>>>>> > >>>>>>>>>> If you fix the issue, kindly add following tag as appropriate > >>>>>>>>>> Reported-by: kernel test robot > >>>>>>>>>> Reported-by: Dan Carpenter > >>>>>>>>>> > >>>>>>>>>> New smatch warnings: > >>>>>>>>>> drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' > >>>>>>>>>> > >>>>>>>>>> vim +1093 drivers/vfio/vfio_iommu_type1.c > >>>>>>>>>> > >>>>>>>>>> 73fa0d10d077d9 Alex Williamson 2012-07-31 1071 static int vfio_dma_do_unmap(struct vfio_iommu *iommu, > >>>>>>>>>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1072 struct vfio_iommu_type1_dma_unmap *unmap, > >>>>>>>>>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1073 struct vfio_bitmap *bitmap) > >>>>>>>>>> 73fa0d10d077d9 Alex Williamson 2012-07-31 1074 { > >>>>>>>>>> c086de818dd81c Kirti Wankhede 2016-11-17 1075 struct vfio_dma *dma, *dma_last = NULL; > >>>>>>>>>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1076 size_t unmapped = 0, pgsize; > >>>>>>>>>> 0f53afa12baec8 Steve Sistare 2021-01-29 1077 int ret = -EINVAL, retries = 0; > >>>>>>>>>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1078 unsigned long pgshift; > >>>>>>>>>> 0f53afa12baec8 Steve Sistare 2021-01-29 1079 dma_addr_t iova = unmap->iova; > >>>>>>>>>> 0f53afa12baec8 Steve Sistare 2021-01-29 1080 unsigned long size = unmap->size; > >>>>>>>>>> ^^^^^^^^^^^^^^^^^^ > >>>>>>>>>> > >>>>>>>>>> 73fa0d10d077d9 Alex Williamson 2012-07-31 1081 > >>>>>>>>>> cade075f265b25 Kirti Wankhede 2020-05-29 1082 mutex_lock(&iommu->lock); > >>>>>>>>>> cade075f265b25 Kirti Wankhede 2020-05-29 1083 > >>>>>>>>>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1084 pgshift = __ffs(iommu->pgsize_bitmap); > >>>>>>>>>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1085 pgsize = (size_t)1 << pgshift; > >>>>>>>>>> cade075f265b25 Kirti Wankhede 2020-05-29 1086 > >>>>>>>>>> 0f53afa12baec8 Steve Sistare 2021-01-29 1087 if (iova & (pgsize - 1)) > >>>>>>>>>> cade075f265b25 Kirti Wankhede 2020-05-29 1088 goto unlock; > >>>>>>>>>> cade075f265b25 Kirti Wankhede 2020-05-29 1089 > >>>>>>>>>> 0f53afa12baec8 Steve Sistare 2021-01-29 1090 if (!size || size & (pgsize - 1)) > >>>>>>>>>> cade075f265b25 Kirti Wankhede 2020-05-29 1091 goto unlock; > >>>>>>>>>> 73fa0d10d077d9 Alex Williamson 2012-07-31 1092 > >>>>>>>>>> 0f53afa12baec8 Steve Sistare 2021-01-29 @1093 if (iova + size - 1 < iova || size > SIZE_MAX) > >>>>>>>>>> > >>>>>>>>>> size is unsigned long and SIZE_MAX is ULONG_MAX so "size > SIZE_MAX" > >>>>>>>>>> does not make sense. > >>>>>>>>> > >>>>>>>>> I think it made sense before the above commit, where unmap->size is a > >>>>>>>>> __u64 and a user could provide a value that exceeds SIZE_MAX on ILP32. > >>>>>>>>> Seems like the fix is probably to use a size_t for the local variable > >>>>>>>>> and restore this test to compare (unmap->size > SIZE_MAX). Steve? > >>>>>>>> > >>>>>>>> Actually it seems like VFIO_DMA_UNMAP_FLAG_ALL doesn't work when > >>>>>>>> PHYS_ADDR_MAX != SIZE_MAX (ex. x86 PAE - I think). > >>>>>>> > >>>>>>> It seems like PAE causes problems even before VFIO_DMA_UNMAP_FLAG_ALL. > >>>>>> > >>>>>> This wouldn't surprise me, I don't know of any actual non-64bit users > >>>>>> and pure 32bit support was only lightly validated ages ago. > >>>>>> > >>>>>>> In the previous vfio_dma_do_unmap code, the u64 unmap->size would be > >>>>>>> truncated when passed to vfio_find_dma. > >>>>>> > >>>>>> We would have failed with -EINVAL before we get there due to this > >>>>>> SIZE_MAX test. I think the existing (previous) PAE interface is at > >>>>>> least self consistent; I see the mapping path also attempts to check > >>>>>> that casting map->size as size_t still matches the original value. > >>>>> > >>>>> Good point, and it also checks for vaddr and iova overflow and wrap: > >>>>> > >>>>> vfio_dma_do_map() > >>>>> if (map->size != size || map->vaddr != vaddr || map->iova != iova) > >>>>> return -EINVAL; > >>>>> if (iova + size - 1 < iova || vaddr + size - 1 < vaddr) { > >>>>> ret = -EINVAL; > >>>>> > >>>>> With that, I don't see a problem with PAE, for unmap-all or otherwise. > >>>>> We just need "u64 size" in vfio_dma_do_unmap to avoid the smatch warning. > >>>> > >>>> I'm not convinced. My understanding is that on PAE phys_addr_t is > >>>> 64-bit while size_t is 32-bit. dma_addr_t (iova above) seems to follow > >>>> phys_addr_t. That suggests to me that our {un}map.iova lives in a > >>>> 64-bit address space, but each mapping is limited to 32-bits. The > >>> > >>> OK, the "map->iova != iova" test does not help because dma_addr_t is 64-bit. My bad. > >>> So, I re-propose my fix for unmap-all from previous email. > >>> > >>> I am not keen on proposing a fix for the potential legacy bugs, vfio_find_dma() and > >>> its callers, if no one is reporting bugs and no one uses it with vfio. It has the > >>> potential for regression with no upside. > >> > >> ... but there are no legacy bugs because size is constrained to 32-bits in do_map as > >> you pointed out, so all calls to vfio_find_dma are safe. > > > > Right, all legacy call paths are ok afaict, but the unmap-all flag > > can't reach any mappings if there are none below an iova of SIZE_MAX. > > We should either fix vfio_find_first_dma_node() for this scenario or > > disable unmap-all where this is a possibility. Thanks, > > Changing size to u64 and using U64_MAX as the upper bound should do the trick: Seems reasonable. Want to slap a title, log, and sign-off on that? Thanks, Alex > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 6cf1dad..b1be0a6 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -181,7 +181,7 @@ static struct vfio_dma *vfio_find_dma(struct vfio_iommu *iommu, > } > > static struct rb_node *vfio_find_dma_first_node(struct vfio_iommu *iommu, > - dma_addr_t start, size_t size) > + dma_addr_t start, u64 size) > { > struct rb_node *res = NULL; > struct rb_node *node = iommu->dma_list.rb_node; > @@ -1184,7 +1184,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, > int ret = -EINVAL, retries = 0; > unsigned long pgshift; > dma_addr_t iova = unmap->iova; > - unsigned long size = unmap->size; > + u64 size = unmap->size; > bool unmap_all = unmap->flags & VFIO_DMA_UNMAP_FLAG_ALL; > bool invalidate_vaddr = unmap->flags & VFIO_DMA_UNMAP_FLAG_VADDR; > struct rb_node *n, *first_n; > @@ -1200,14 +1200,12 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, > if (unmap_all) { > if (iova || size) > goto unlock; > - size = SIZE_MAX; > - } else if (!size || size & (pgsize - 1)) { > + size = U64_MAX; > + } else if (!size || size & (pgsize - 1) || > + iova + size - 1 < iova || size > SIZE_MAX) { > goto unlock; > } > > - if (iova + size - 1 < iova || size > SIZE_MAX) > - goto unlock; > - > /* When dirty tracking is enabled, allow only min supported pgsize */ > if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) && > (!iommu->dirty_page_tracking || (bitmap->pgsize != pgsize))) { >