gntdev: misc cleanup - eliminate struct gntdev_grant_info's dev_bus_addr member (which was used to communicate a value inside the main loop of gntdev_mmap()) - correct types - use a local variable 'grants' in gntdev_mmap() to improve readability - avoid re-calculating already calculated values - properly check for out of range values - combine both GNTTABOP_unmap_grant_ref calls in gntdev_clear_pte() into a single hypercall - adjust error diagnostic in unmap ioctl to be more useful and better legible Signed-off-by: Jan Beulich --- a/drivers/xen/gntdev/gntdev.c +++ b/drivers/xen/gntdev/gntdev.c @@ -80,7 +80,6 @@ typedef struct gntdev_grant_info { grant_ref_t ref; grant_handle_t kernel_handle; grant_handle_t user_handle; - uint64_t dev_bus_addr; } valid; } u; } gntdev_grant_info_t; @@ -473,14 +472,14 @@ static int gntdev_mmap (struct file *fli { struct gnttab_map_grant_ref op; unsigned long slot_index = vma->vm_pgoff; - unsigned long kernel_vaddr, user_vaddr; - uint32_t size = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; + unsigned long kernel_vaddr, user_vaddr, mfn; + unsigned long size = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT; uint64_t ptep; int ret, exit_ret; - int flags; - int i; + unsigned int i, flags; struct page *page; gntdev_file_private_data_t *private_data = flip->private_data; + gntdev_grant_info_t *grants; struct vm_foreign_map *foreign_map; if (unlikely(!private_data)) { @@ -490,17 +489,19 @@ static int gntdev_mmap (struct file *fli /* Test to make sure that the grants array has been initialised. */ down_read(&private_data->grants_sem); - if (unlikely(!private_data->grants)) { - up_read(&private_data->grants_sem); + grants = private_data->grants; + up_read(&private_data->grants_sem); + + if (unlikely(!grants)) { printk(KERN_ERR "Attempted to mmap before ioctl.\n"); return -EINVAL; } - up_read(&private_data->grants_sem); + grants += slot_index; - if (unlikely((size <= 0) || - (size + slot_index) > private_data->grants_size)) { + if (unlikely(size + slot_index <= slot_index || + size + slot_index > private_data->grants_size)) { printk(KERN_ERR "Invalid number of pages or offset" - "(num_pages = %d, first_slot = %ld).\n", + "(num_pages = %lu, first_slot = %lu)\n", size, slot_index); return -ENXIO; } @@ -521,11 +522,10 @@ static int gntdev_mmap (struct file *fli /* Slots must be in the NOT_YET_MAPPED state. */ down_write(&private_data->grants_sem); for (i = 0; i < size; ++i) { - if (private_data->grants[slot_index + i].state != - GNTDEV_SLOT_NOT_YET_MAPPED) { - printk(KERN_ERR "Slot (index = %ld) is in the wrong " + if (grants[i].state != GNTDEV_SLOT_NOT_YET_MAPPED) { + printk(KERN_ERR "Slot (index = %lu) is in the wrong " "state (%d).\n", slot_index + i, - private_data->grants[slot_index + i].state); + grants[i].state); up_write(&private_data->grants_sem); kfree(foreign_map); return -EINVAL; @@ -565,14 +565,11 @@ static int gntdev_mmap (struct file *fli flags |= GNTMAP_readonly; kernel_vaddr = get_kernel_vaddr(private_data, slot_index + i); - user_vaddr = get_user_vaddr(vma, i); page = private_data->foreign_pages[slot_index + i]; gnttab_set_map_op(&op, kernel_vaddr, flags, - private_data->grants[slot_index+i] - .u.valid.ref, - private_data->grants[slot_index+i] - .u.valid.domid); + grants[i].u.valid.ref, + grants[i].u.valid.domid); /* Carry out the mapping of the grant reference. */ ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, @@ -583,10 +580,8 @@ static int gntdev_mmap (struct file *fli printk(KERN_ERR "Error mapping the grant reference " "into the kernel (%d). domid = %d; ref = %d\n", op.status, - private_data->grants[slot_index+i] - .u.valid.domid, - private_data->grants[slot_index+i] - .u.valid.ref); + grants[i].u.valid.domid, + grants[i].u.valid.ref); else /* Propagate eagain instead of trying to fix it up */ exit_ret = -EAGAIN; @@ -597,16 +592,14 @@ static int gntdev_mmap (struct file *fli SetPageReserved(page); /* Record the grant handle, for use in the unmap operation. */ - private_data->grants[slot_index+i].u.valid.kernel_handle = - op.handle; - private_data->grants[slot_index+i].u.valid.dev_bus_addr = - op.dev_bus_addr; + grants[i].u.valid.kernel_handle = op.handle; - private_data->grants[slot_index+i].state = GNTDEV_SLOT_MAPPED; - private_data->grants[slot_index+i].u.valid.user_handle = - GNTDEV_INVALID_HANDLE; + grants[i].state = GNTDEV_SLOT_MAPPED; + grants[i].u.valid.user_handle = GNTDEV_INVALID_HANDLE; /* Now perform the mapping to user space. */ + user_vaddr = get_user_vaddr(vma, i); + if (xen_feature(XENFEAT_auto_translated_physmap)) { /* USING SHADOW PAGE TABLES. */ /* In this case, we simply insert the page into the VM @@ -622,11 +615,11 @@ static int gntdev_mmap (struct file *fli /* In this case, we map the grant(s) straight into user * space. */ + mfn = op.dev_bus_addr >> PAGE_SHIFT; /* Get the machine address of the PTE for the user page. */ if ((ret = create_lookup_pte_addr(vma->vm_mm, - vma->vm_start - + (i << PAGE_SHIFT), + user_vaddr, &ptep))) { printk(KERN_ERR "Error obtaining PTE pointer (%d)\n", @@ -636,9 +629,6 @@ static int gntdev_mmap (struct file *fli /* Configure the map operation. */ - /* The reference is to be used by host CPUs. */ - flags = GNTMAP_host_map; - /* Specifies a user space mapping. */ flags |= GNTMAP_application_map; @@ -647,14 +637,9 @@ static int gntdev_mmap (struct file *fli */ flags |= GNTMAP_contains_pte; - if (!(vma->vm_flags & VM_WRITE)) - flags |= GNTMAP_readonly; - gnttab_set_map_op(&op, ptep, flags, - private_data->grants[slot_index+i] - .u.valid.ref, - private_data->grants[slot_index+i] - .u.valid.domid); + grants[i].u.valid.ref, + grants[i].u.valid.domid); /* Carry out the mapping of the grant reference. */ ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, @@ -664,10 +649,8 @@ static int gntdev_mmap (struct file *fli printk(KERN_ERR "Error mapping the grant reference " "into user space (%d). domid = %d; ref = %d\n", op.status, - private_data->grants[slot_index+i].u - .valid.domid, - private_data->grants[slot_index+i].u - .valid.ref); + grants[i].u.valid.domid, + grants[i].u.valid.ref); /* This should never happen after we've mapped into * the kernel space. */ BUG_ON(op.status == GNTST_eagain); @@ -677,15 +660,11 @@ static int gntdev_mmap (struct file *fli /* Record the grant handle, for use in the unmap * operation. */ - private_data->grants[slot_index+i].u.valid.user_handle - = op.handle; + grants[i].u.valid.user_handle = op.handle; /* Update p2m structure with the new mapping. */ set_phys_to_machine(__pa(kernel_vaddr) >> PAGE_SHIFT, - FOREIGN_FRAME(private_data-> - grants[slot_index+i] - .u.valid.dev_bus_addr - >> PAGE_SHIFT)); + FOREIGN_FRAME(mfn)); } exit_ret = 0; @@ -715,9 +694,11 @@ undo_map_out: static pte_t gntdev_clear_pte(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep, int is_fullmm) { - int slot_index, ret; + int ret; + unsigned int nr; + unsigned long slot_index; pte_t copy; - struct gnttab_unmap_grant_ref op; + struct gnttab_unmap_grant_ref op[2]; gntdev_file_private_data_t *private_data; /* THIS IS VERY UNPLEASANT: do_mmap_pgoff() will set the vma->vm_file @@ -750,36 +731,35 @@ static pte_t gntdev_clear_pte(struct vm_ return copy; } - /* First, we clear the user space mapping, if it has been made. - */ + /* Clear the user space mapping, if it has been made. */ if (private_data->grants[slot_index].u.valid.user_handle != - GNTDEV_INVALID_HANDLE && - !xen_feature(XENFEAT_auto_translated_physmap)) { - /* NOT USING SHADOW PAGE TABLES. */ - gnttab_set_unmap_op(&op, ptep_to_machine(ptep), + GNTDEV_INVALID_HANDLE) { + /* NOT USING SHADOW PAGE TABLES (and user handle valid). */ + gnttab_set_unmap_op(&op[0], ptep_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); - BUG_ON(ret); - if (op.status != GNTST_okay) - printk("User unmap grant status = %d\n", op.status); + nr = 1; } else { - /* USING SHADOW PAGE TABLES. */ + /* USING SHADOW PAGE TABLES (or user handle invalid). */ pte_clear_full(vma->vm_mm, addr, ptep, is_fullmm); + nr = 0; } - /* Finally, we unmap the grant from kernel space. */ - gnttab_set_unmap_op(&op, + /* We always unmap the grant from kernel space. */ + gnttab_set_unmap_op(&op[nr], get_kernel_vaddr(private_data, 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); + + ret = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, op, nr + 1); BUG_ON(ret); - if (op.status != GNTST_okay) - printk("Kernel unmap grant status = %d\n", op.status); + + if (nr && op[0].status != GNTST_okay) + printk("User unmap grant status = %d\n", op[0].status); + if (op[nr].status != GNTST_okay) + printk("Kernel unmap grant status = %d\n", op[nr].status); /* Return slot to the not-yet-mapped state, so that it may be * mapped again, or removed by a subsequent ioctl. @@ -915,7 +895,8 @@ private_data_initialised: return -EFAULT; start_index = op.index >> PAGE_SHIFT; - if (start_index + op.count > private_data->grants_size) + if (start_index + op.count < start_index || + start_index + op.count > private_data->grants_size) return -EINVAL; down_write(&private_data->grants_sem); @@ -924,28 +905,29 @@ private_data_initialised: * state. */ for (i = 0; i < op.count; ++i) { - if (unlikely - (private_data->grants[start_index + i].state - != GNTDEV_SLOT_NOT_YET_MAPPED)) { - if (private_data->grants[start_index + i].state - == GNTDEV_SLOT_INVALID) { - printk(KERN_ERR - "Tried to remove an invalid " - "grant at offset 0x%x.", - (start_index + i) - << PAGE_SHIFT); - rc = -EINVAL; - } else { - printk(KERN_ERR - "Tried to remove a grant which " - "is currently mmap()-ed at " - "offset 0x%x.", - (start_index + i) - << PAGE_SHIFT); - rc = -EBUSY; - } - goto unmap_out; + const char *what; + + switch (private_data->grants[start_index + i].state) { + case GNTDEV_SLOT_NOT_YET_MAPPED: + continue; + case GNTDEV_SLOT_INVALID: + what = "invalid"; + rc = -EINVAL; + break; + case GNTDEV_SLOT_MAPPED: + what = "currently mmap()-ed"; + rc = -EBUSY; + break; + default: + what = "in an invalid state"; + rc = -ENXIO; + break; } + printk(KERN_ERR "%s[%d] tried to remove a grant" + " which is %s at %#x+%#x\n", + current->comm, current->pid, + what, start_index, i); + goto unmap_out; } down_write(&private_data->free_list_sem);