Re: [PATCH] xen/grant-table: correctly initialize grant table version 1

[Matt Wilson <msw@amazon.com>]

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