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
|
Home