Re: [Xen-devel] possible grant table issue

[Stefan Berger <stefanb@xxxxxxxxxx>]

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

gt_fix.patch
Description: Binary data

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