From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Wilson Subject: Re: [PATCH] xen/grant-table: correctly initialize grant table version 1 Date: Thu, 20 Dec 2012 13:19:47 -0800 Message-ID: <20121220211945.GA15542@u109add4315675089e695.ant.amazon.com> References: <1355948414-7503-1-git-send-email-msw@amazon.com> <1355997710.26722.12.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1355997710.26722.12.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Annie Li , Steven Noonan , "xen-devel@lists.xen.org" , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On Thu, Dec 20, 2012 at 10:01:50AM +0000, Ian Campbell wrote: > On Wed, 2012-12-19 at 20:20 +0000, Matt Wilson wrote: > > Commit 85ff6acb075a484780b3d763fdf41596d8fc0970 (xen/granttable: Grant > > tables V2 implementation) changed the GREFS_PER_GRANT_FRAME macro from > > a constant to a conditional expression. The expression depends on > > grant_table_version being appropriately set. Unfortunately, at init > > time grant_table_version will be 0. The GREFS_PER_GRANT_FRAME > > conditional expression checks for "grant_table_version == 1", and > > therefore returns the number of grant references per frame for v2. > > > > This causes gnttab_init() to allocate fewer pages for gnttab_list, as > > a frame can old half the number of v2 entries than v1 entries. After > > gnttab_resume() is called, grant_table_version is appropriately > > set. nr_init_grefs will then be miscalculated and gnttab_free_count > > will hold a value larger than the actual number of free gref entries. > > > > If a guest is heavily utilizing improperly initialized v1 grant > > tables, memory corruption can occur. > > Good catch! > > [...] > @@ -1080,10 +1081,9 @@ static void gnttab_request_version(void) > > panic("we need grant tables version 2, but only version 1 is available"); > > } else { > > grant_table_version = 1; > > + grefs_per_grant_frame = PAGE_SIZE / sizeof(struct grant_entry_v1); > > gnttab_interface = &gnttab_v1_ops; > > } > > - printk(KERN_INFO "Grant tables using version %d layout.\n", > > - grant_table_version); > > You remove this here, and re-add it in the gnttab_resume callsite but I > don't see anything added at the gnttab_init callsite. It would be > useful to keep this print at start of day too. > > Oh, I see, gnttab_init also calls gnttab_resume later so we get the > message from there, with gnttab_request_version getting called twice. Right, I was just avoiding printing the "Grant tables using version 1 layout." message at init time. > Can we avoid doing this twice by moving the tail of gnttab_resume > (everything except the gnttab_request_version) into a new gnttab_setup > and calling that from gnttab_resume and gnttab_init (instead of calling > resume)? Sure, I can do that. Matt