From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Subject: Re: xen/mmu: Copy and revector the P2M tree. Date: Fri, 7 Aug 2015 03:23:02 +0300 Message-ID: <20150807002301.GG5096@mwanda> References: <20150720100352.GA4084@mwanda> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZNVRb-0007hk-Fr for xen-devel@lists.xenproject.org; Fri, 07 Aug 2015 00:23:31 +0000 Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by userp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id t770NRNv022209 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 7 Aug 2015 00:23:27 GMT Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userv0021.oracle.com (8.13.8/8.13.8) with ESMTP id t770NQAD019631 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Fri, 7 Aug 2015 00:23:27 GMT Received: from abhmp0010.oracle.com (abhmp0010.oracle.com [141.146.116.16]) by aserv0122.oracle.com (8.13.8/8.13.8) with ESMTP id t770NQh2021210 for ; Fri, 7 Aug 2015 00:23:26 GMT Content-Disposition: inline In-Reply-To: <20150720100352.GA4084@mwanda> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: konrad.wilk@oracle.com Cc: xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Mon, Jul 20, 2015 at 01:03:52PM +0300, Dan Carpenter wrote: > Hello Konrad Rzeszutek Wilk, > > The patch 7f9140626c75: "xen/mmu: Copy and revector the P2M tree." > from Jul 26, 2012, leads to the following static checker warning: > > arch/x86/xen/mmu.c:1105 xen_cleanhighmap() > warn: potential pointer math issue ('level2_kernel_pgt' is pointer to unsigned long) > > arch/x86/xen/mmu.c > 1096 #ifdef CONFIG_X86_64 > 1097 static void __init xen_cleanhighmap(unsigned long vaddr, > 1098 unsigned long vaddr_end) > 1099 { > 1100 unsigned long kernel_end = roundup((unsigned long)_brk_end, PMD_SIZE) - 1; > 1101 pmd_t *pmd = level2_kernel_pgt + pmd_index(vaddr); > 1102 > 1103 /* NOTE: The loop is more greedy than the cleanup_highmap variant. > 1104 * We include the PMD passed in on _both_ boundaries. */ > 1105 for (; vaddr <= vaddr_end && (pmd < (level2_kernel_pgt + PAGE_SIZE)); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > This pointer math is weird because we typically think of PAGE_SIZE as > a number of bytes but since level2_kernel_pgt is a pointer to unsigned > long, it looks like this loop can go through more iterations than > intended. > Yeah. This condition doesn't make sense at all. level2_kernel_pgt is an array that has 512 elements but PAGE_SIZE is 4096 so we would be well past the end of the array. I suspect that this works because it is only called when DEBUG in defined or because of the "vaddr <= vaddr_end" condition. > 1106 pmd++, vaddr += PMD_SIZE) { > 1107 if (pmd_none(*pmd)) > 1108 continue; > 1109 if (vaddr < (unsigned long) _text || vaddr > kernel_end) > 1110 set_pmd(pmd, __pmd(0)); > 1111 } > 1112 /* In case we did something silly, we should crash in this function > 1113 * instead of somewhere later and be confusing. */ > 1114 xen_mc_flush(); > 1115 } > > regards, > dan carpenter