[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] possible grant table issue


  • To: Stefan Berger <stefanb@xxxxxxxxxx>
  • From: Christopher Clark <christopher.w.clark@xxxxxxxxx>
  • Date: Fri, 15 Jul 2005 11:58:47 -0700
  • Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
  • Delivery-date: Fri, 15 Jul 2005 18:58:13 +0000
  • Domainkey-signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:message-id:date:from:reply-to:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=I+wCqs0V5EW/3bNfft6AEUYfkUrMWYZ9LSPFecwErhpbosq9W8jD0RvOioAt1TO164Fwyxpw8tmRjehC0CFRHapglHcA/W+vLXqphEB10NhVObM+npFYFzm5aHw41smyutx1kCH5AVI6H4yFsitrfBKzmKdgQlCVtik+rSpe2D4=
  • List-id: Xen developer discussion <xen-devel.lists.xensource.com>

Hi Stefan

Good catch. I vote apply.

Christopher


On 7/15/05, Stefan Berger <stefanb@xxxxxxxxxx> wrote:
>  
> Hello Christopher, 
>  
>   thanks for the explanation. I actually see the strange occurrence in a
> user domain where I have grant table-enabled backend drivers. It happens if
> another domain connecting to the backend drivers is killed and possibly due
> to some timing issues with XEN-D the unmapping of the grant tables does not
> happen through the HYPERVISOR_grant_table_op first, but through some
> cleaning up after the dying domain which places a call to 
>  
> int
>  gnttab_check_unmap(
>     struct domain *rd, struct domain *ld, unsigned long frame, int readonly)
>  
> [called from put_page_from_l1e()] and selects the wrong handle (=0) there.
> Only after this happens does the backend receive the destroy message from
> XEN-D and wants to clean up the grant table with the hypervisor call using
> the correct handle, but fails. 
>  
> The fix for this is attached. The problem is that the selected map is not
> tested whether it was created for the (remote) domain 'rd'. 
>  
>   Stefan 
>  
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxx> 
>  
> 
>  
>   
>  
>  
> xen-devel-bounces@xxxxxxxxxxxxxxxxxxx wrote on 07/13/2005
> 04:16:10 AM:
> 
>  
>  > Hi Stefan
>  > 
>  > Here's how it works:
>  > 
>  > maptrack_head is the index of the next free entry in the maptrack
>  > array. A handle is just an index into the array.
>  >                                                                          
>    
>  > Ignoring the shift, grant_table->maptrack array entries that are not in
> use
>  > contain the index of the next free entry, forming a list and
>  > terminating with a sentinel value. When entries are freed, the array
>  > contents are overwritten in order to add the entry to the free list.
>  >                                                                          
>    
>  > Thus the grant_table->maptrack[index] entries are initialised to an
> increasing
>  > series starting from 1 at index 0, and maptrack_head = 0. The sentinel
>  > value is the number of elements in the array.
>  >                                                                          
>    
>  > So the sequence you describe will result in:
>  >                                                                          
>    
>  > get_maptrack_handle(t)
>  > => maptrack_head is now 1
>  > => returns handle of 0
>  > => t->maptrack_entry[0] in use
>  >                                                                          
>    
>  > get_maptrack_handle(t)
>  > => maptrack_head is now 2
>  > => returns handle of 1
>  > => t->maptrack_entry[1] in use
>  >                                                                          
>    
>  > get_maptrack_handle(t)
>  > => maptrack_head is now 3
>  > => returns handle of 2
>  > => t->maptrack_entry[2] in use
>  >                                                                          
>    
>  > put_maptrack_handle(t, 2)
>  > => t->maptrack_entry[2] points to next free index 3
>  > => maptrack_head is now 2
>  >                                                                          
>    
>  > get_maptrack_handle(t)
>  > => maptrack_head is now 3
>  > => returns handle of 2
>  > => t->maptrack_entry[2] in use
>  > 
>  > You shouldn't be returned a handle of 0 when getting a handle
>  > immediately after freeing handle 2.
>  > 
>  > Christopher
>  > 
>  > 
>  > On 7/12/05, Stefan Berger <stefanb@xxxxxxxxxx> wrote:
>  > > Hello!
>  > > 
>  > > Attached is a patch that dumps some debugging output for the block
>  > > interface backend. The reason why I am posting this patch is due to the
>  > > somewhat strange assignments of the handles that are returned from the
>  > > HYPERVISOR_grant_table_op. I am stopping short of saying it's a bug,
>  > > because I don't know the code well enough, but when looking at the
>  > > hypervisor code I see some place where I doubt that this is right.
>  > > Particularly one should try the following:
>  > > 
>  > > Create user domains that use the block interfaces.
>  > > 
>  > > 1st user domain witll be assigned handle 0x0. - should be ok
>  > > 2nd user domain will be assigned handle 0x1. - should be ok
>  > > 3rd user domain will be assigned handle 0x2. - should be ok
>  > > 
>  > > (handle numbers have obviously been increasing so far)
>  > > 
>  > > bring down 3rd user domain - free'ed handle will be 0x2 - should be ok
>  > > 
>  > > create 3rd user domain again - will be assigned handle 0x0 - this is
> not
>  > > what I would expect.
>  > > 
>  > > (the code that's causing this is called when handle 0x2 was free'ed
>  > > static inline void
>  > > put_maptrack_handle(
>  > >             grant_table_t *t, int handle)
>  > >         {
>  > >             t->maptrack[handle].ref_and_flags =
> t->maptrack_head <<
>  > > MAPTRACK_REF_SHIFT;
>  > >             t->maptrack_head = handle;
>  > >                              ^^^^^^
>  > >             t->map_count--;
>  > >         }
>  > > )
>  > > 
>  > > 
>  > > 
>  > > Now when I look  at xen/common/grant_tables.c I see how the handles are
>  > > used in :
>  > > 
>  > > 
>  > > static int
>  > > __gnttab_map_grant_ref(
>  > >     gnttab_map_grant_ref_t *uop,
>  > >     unsigned long *va)
>  > > {
>  > >         [...] // much omitted
>  > > 
>  > >     if ( 0 <= ( rc = __gnttab_activate_grant_ref( ld, led, rd, ref,
>  > >                                                   dev_hst_ro_flags,
>  > >                                                   host_virt_addr,
>  > > &frame)))
>  > >     {
>  > >         /*
>  > >          * Only make the maptrack live _after_ writing the pte, in case
> we
>  > > 
>  > >          * overwrite the same frame number, causing a maptrack walk to
>  > > find it
>  > >          */
>  > >         ld->grant_table->maptrack[handle].domid = dom;
>  > >                                   ^^^^^^
>  > >        
> ld->grant_table->maptrack[handle].ref_and_flags
>  > >                                   ^^^^^^
>  > >             = (ref << MAPTRACK_REF_SHIFT) |
>  > >               (dev_hst_ro_flags & MAPTRACK_GNTMAP_MASK);
>  > > 
>  > >         (void)__put_user(frame, &uop->dev_bus_addr);
>  > > 
>  > >         if ( dev_hst_ro_flags & GNTMAP_host_map )
>  > >             *va = host_virt_addr;
>  > > 
>  > >         (void)__put_user(handle, &uop->handle);
>  > > 
>  > > 
>  > > I think this newly assigned handle of '0' (for the re-created 3rd user
>  > > domain) is overwriting some previously assign array entry for the first
>  > > user domain. Please someone who knows have a look at this. All this is
>  > > happening in the domain where the blockdevice backend is located.
>  > > 
>  > >    Stefan
>  > > 
>  > > 
>  > > Signed-off-by : Stefan Berger <stefanb@xxxxxxxxxx>
>  > > 
>  > > 
>  > > 
>  > > _______________________________________________
>  > > Xen-devel mailing list
>  > > Xen-devel@xxxxxxxxxxxxxxxxxxx
>  > > http://lists.xensource.com/xen-devel
>  > > 
>  > > 
>  > > 
>  > >
>  > 
>  > _______________________________________________
>  > Xen-devel mailing list
>  > Xen-devel@xxxxxxxxxxxxxxxxxxx
>  > http://lists.xensource.com/xen-devel
>  
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.