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 77325C433E0 for ; Tue, 23 Feb 2021 17:45:54 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D9A9864E83 for ; Tue, 23 Feb 2021 17:45:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D9A9864E83 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 61D806B0005; Tue, 23 Feb 2021 12:45:53 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5A5986B0006; Tue, 23 Feb 2021 12:45:53 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 445918D0001; Tue, 23 Feb 2021 12:45:53 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0253.hostedemail.com [216.40.44.253]) by kanga.kvack.org (Postfix) with ESMTP id 28C336B0005 for ; Tue, 23 Feb 2021 12:45:53 -0500 (EST) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id E0895180275BC for ; Tue, 23 Feb 2021 17:45:52 +0000 (UTC) X-FDA: 77850260544.04.D6D9F2F Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by imf24.hostedemail.com (Postfix) with ESMTP id 82EABA0002DF for ; Tue, 23 Feb 2021 17:45:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1614102344; 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=m2v4Vbm+PqtuNCqpSOXvlIe3oe35G49vhSikqVZH69A=; b=fpVB2nsa5cFtLmsaJNgNa8ckZ46GWEYC12KCZ54o/O8bcQKf2RrepYk5vJmin80fTTSYVg NdAirl8WOHmJH72wW26EPz31frK7PWAbjOJk9wpxe7lDPd0RUWSUbaaaOqWrUZPgAihlAH HZk6aamwDqCk/8t6eqtAzn0Rj1hsYy4= 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-528-8-b5PewHPzay5YeAArkbnA-1; Tue, 23 Feb 2021 12:45:38 -0500 X-MC-Unique: 8-b5PewHPzay5YeAArkbnA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 23510100CD19; Tue, 23 Feb 2021 17:45:37 +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 0B5C35D6A1; Tue, 23 Feb 2021 17:45:35 +0000 (UTC) Date: Tue, 23 Feb 2021 10:45:35 -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: <20210223104535.17986dee@omen.home.shazbot.org> In-Reply-To: References: <20210222141043.GW2222@kadam> <20210222155145.50e2d513@omen.home.shazbot.org> <20210222161753.7acc4e92@omen.home.shazbot.org> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 82EABA0002DF X-Stat-Signature: yay5udntbbrw9ur5r44gx491inugxiyj Received-SPF: none (redhat.com>: No applicable sender policy available) receiver=imf24; identity=mailfrom; envelope-from=""; helo=us-smtp-delivery-124.mimecast.com; client-ip=63.128.21.124 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1614102346-35367 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 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. > For unmap, these fixes should suffice, and I would rather do this than > disable the unmap-all flag for a corner case: > > vfio_dma_do_unmap() > size_t unmapped = 0; > unsigned long size = unmap->size; > ==> > u64 unmapped = 0; > u64 size = unmap->size; > > static struct rb_node *vfio_find_dma_first_node( > struct vfio_iommu *iommu, dma_addr_t start, size_t size) > ==> > static struct rb_node *vfio_find_dma_first_node( > struct vfio_iommu *iommu, dma_addr_t start, u64 size) > > And maybe use dma_addr_t instead of u64 in the above (which is 64 bits for > CONFIG_X86_PAE). > > However, there are other places in the existing code that need tweaking > to be safe for PAE, the vfio_find_dma() size arg for one. Yes, it looks like the IOMMU aperture checking using vfio_find_dma() could have issues where dma_addr_t > size_t. Do you want to propose a patch? Thanks, Alex > > I can't say I'm > > really interested in adding complexity to make it work in such a case > > either. Maybe we can just not expose it, ex: > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > > index ed03f3fcb07e..6b69a74b3db0 100644 > > --- a/drivers/vfio/vfio_iommu_type1.c > > +++ b/drivers/vfio/vfio_iommu_type1.c > > @@ -1207,7 +1207,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; > > + size_t 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; > > @@ -1228,7 +1228,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, > > goto unlock; > > } > > > > - if (iova + size - 1 < iova || size > SIZE_MAX) > > + if (iova + size - 1 < iova || unmap->size > SIZE_MAX) > > goto unlock; > > > > /* When dirty tracking is enabled, allow only min supported pgsize */ > > @@ -2657,9 +2657,10 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu, > > case VFIO_TYPE1_IOMMU: > > case VFIO_TYPE1v2_IOMMU: > > case VFIO_TYPE1_NESTING_IOMMU: > > - case VFIO_UNMAP_ALL: > > case VFIO_UPDATE_VADDR: > > return 1; > > + case VFIO_UNMAP_ALL: > > + return PHYS_ADDR_MAX == SIZE_MAX ? 1 : 0; > > case VFIO_DMA_CC_IOMMU: > > if (!iommu) > > return 0; > > @@ -2868,6 +2869,10 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu, > > VFIO_DMA_UNMAP_FLAG_VADDR))) > > return -EINVAL; > > > > + if ((PHYS_ADDR_MAX != SIZE_MAX) && > > + (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL)) > > + return -EINVAL; > > + > > if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) { > > unsigned long pgshift; > > > > > > > > > > > >>> Is the " - 1" intentional on the other overflow check? As in it's okay > >>> to wrap around to zero but not further than that? Sometimes this is > >>> intentional but it requires more subsystem expertise than I possess. > >> > >> Yes, since we're dealing with a start + length we need to account for > >> the -1 in the end value, otherwise the user could never unmap the last > >> page of the address space. Thanks for the report! > >> > >> Alex > >> > >>> cade075f265b25 Kirti Wankhede 2020-05-29 1094 goto unlock; > >>> 73fa0d10d077d9 Alex Williamson 2012-07-31 1095 > >>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1096 /* When dirty tracking is enabled, allow only min supported pgsize */ > >>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1097 if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) && > >>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1098 (!iommu->dirty_page_tracking || (bitmap->pgsize != pgsize))) { > >>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1099 goto unlock; > >>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1100 } > >>> 73fa0d10d077d9 Alex Williamson 2012-07-31 1101 > >>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1102 WARN_ON((pgsize - 1) & PAGE_MASK); > >>> 331e33d2960c82 Kirti Wankhede 2020-05-29 1103 again: > >>> 1ef3e2bc04223f Alex Williamson 2014-02-26 1104 /* > >>> > >>> --- > >>> 0-DAY CI Kernel Test Service, Intel Corporation > >>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org > >> > > >