WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] [PATCH 2/3] User-space grant table device - main driver

To: Derek Murray <Derek.Murray@xxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 2/3] User-space grant table device - main driver
From: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
Date: Thu, 29 Mar 2007 15:26:51 +0900
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
Delivery-date: Wed, 28 Mar 2007 23:25:47 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <68F773A4-30BF-49BE-A3CB-8551D223AC0F@xxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <68F773A4-30BF-49BE-A3CB-8551D223AC0F@xxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
On Wed, Mar 28, 2007 at 01:56:16PM +0100, Derek Murray wrote:

> +static int gntdev_mmap (struct file *flip, struct vm_area_struct *vma) 
> +{
...
> +                     /* In this case, we simply insert the page into the VM
> +                      * area. */
> +                     ret = vm_insert_page(vma, user_vaddr, page);
...
> +undo_map_out:
> +
> +     /* If we have a mapping failure during the mmap, we rollback all
> +      * mappings. 
> +      */
> +
> +     /* First undo the kernel mappings. */
> +     for (i = 0; i < undo_count_kmaps; ++i) {
> +             struct gnttab_unmap_grant_ref unmap_op; 
> +             gnttab_set_unmap_op(&unmap_op, 
> +                                 get_kernel_vaddr(vma, slot_index + i),
> +                                 GNTMAP_host_map,
> +                                 private_data->grants[slot_index]
> +                                 .u.valid.kernel_handle);
> +             ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, 
> +                                             &unmap_op, 1);
> +             BUG_ON(ret);
> +
> +             printk("Kernel unmap status = %d\n", unmap_op.status);
> +
> +             /* Return slot to the not-yet-mapped state, so that it may be
> +              * mapped again, or removed by a subsequent ioctl.
> +              */
> +             private_data->grants[slot_index+i].state =
> +                     GNTDEV_SLOT_NOT_YET_MAPPED;
> +
> +             /* Invalidate the physical to machine mapping for this page. */
> +             set_phys_to_machine(__pa(get_kernel_vaddr(vma, slot_index)) 
> +                                 >> PAGE_SHIFT, INVALID_P2M_ENTRY);
> +     }

This doesn't undo vm_insert_page() when auto trasnalted physmap mode.
Probably just calling zap_page_range() would be OK instead
of undo_count_kmap loop.


> +static pte_t gntdev_clear_pte(struct vm_area_struct *vma, unsigned long addr,
> +                           pte_t *ptep, int is_fullmm)
> +{
> +     int slot_index, ret;
> +     pte_t copy;
> +     struct gnttab_unmap_grant_ref op;
> +     gntdev_file_private_data_t *private_data 
> +             = (gntdev_file_private_data_t *) vma->vm_file->private_data;
> +
> +     /* Copy the existing value of the PTE for returning. */
> +     copy = *ptep;
> +
> +     /* Calculate the grant relating to this PTE. */
> +     slot_index = vma->vm_pgoff + ((addr - vma->vm_start) >> PAGE_SHIFT);
> +
> +     /* First, we unmap the grant from kernel space. */
> +     gnttab_set_unmap_op(&op, get_kernel_vaddr(vma, slot_index),
> +                         GNTMAP_host_map, 
> +                         private_data->grants[slot_index].u.valid
> +                         .kernel_handle);
> +     ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1);
> +        BUG_ON(ret);
> +     if (op.status)
> +             printk("Kernel unmap grant status = %d\n", op.status);
> +
> +     /* Next, we clear the user space mapping. */
> +     if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> +             /* NOT USING SHADOW PAGE TABLES. */
> +             gnttab_set_unmap_op(&op, virt_to_machine(ptep), 
> +                                 GNTMAP_contains_pte,
> +                                 private_data->grants[slot_index].u.valid
> +                                 .user_handle);
> +             ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, 
> +                                             &op, 1);
> +             if (op.status)
> +                     printk("User unmap grant status = %d\n", op.status);
> +             BUG_ON(ret);
> +     } else {
> +             /* USING SHADOW PAGE TABLES. */
> +             pte_clear_full(vma->vm_mm, addr, ptep, is_fullmm);

Please clear the entry before grant table unmap.
If the virtual address is accessed between grant tabel unmap and
pte_clear_full(), something bad would happen.
The higher level exclusion is done and I might be somewhat paranoia, though.


> +/* Called when an ioctl is made on the device.
> + */
> +static int gntdev_ioctl(struct inode *inode, struct file *flip,
> +                     unsigned int cmd, unsigned long arg)
> +{
> +     int rc = 0;
> +     switch (cmd) {
> +     case IOCTL_GNTDEV_MAP_GRANT_REF:
> +     {
> +             struct ioctl_gntdev_map_grant_ref op;
> +             struct ioctl_gntdev_grant_ref ref;
> +             down_write(&current->mm->mmap_sem);
> +
> +             if ((rc = copy_from_user(&op, 
> +                                      (void __user *) arg, 
> +                                      sizeof(op)))) {

copy_from/to_user() may result in page fault and page fault handler
may try to obtain it causing dead lock.

-- 
yamahata

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