Re: [PATCH 26/27] Enable 32bit dirty log pointers on 64bit host

[Avi Kivity <avi@redhat.com>]

On 09/30/2009 02:04 PM, Arnd Bergmann wrote:
> On Tuesday 29 September 2009, Avi Kivity wrote:
>    
>>>          r = -EINVAL;
>>>          if (log->slot>= KVM_MEMORY_SLOTS)
>>> @@ -718,8 +719,15 @@ int kvm_get_dirty_log(struct kvm *kvm,
>>>          for (i = 0; !any&&  i<  n/sizeof(long); ++i)
>>>                  any = memslot->dirty_bitmap[i];
>>>
>>> +#if defined(__BIG_ENDIAN)&&  defined(CONFIG_64BIT)
>>> +       /* Need to convert user pointers */
>>> +       if (test_thread_flag(TIF_32BIT))
>>> +               target_bm = (void*)((u64)log->dirty_bitmap>>  32);
>>> +       else
>>> +#endif
>>> +       target_bm = log->dirty_bitmap;
>>>          r = -EFAULT;
>>> -       if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
>>> +       if (copy_to_user(target_bm, memslot->dirty_bitmap, n))
>>>                  goto out;
>>>
>>>          if (any)
>>>        
>> Ah, that's much better.  Plus a mental note not to put pointers in
>> user-visible structures in the future.  This can serve as a reminder :)
>>      
> It's still broken on s390, which
>
> 1. uses TIF_31BIT instead of TIF_32BIT
> 2. needs to call compat_ptr() to do a real conversion instead of a cast
>
> The TIF_32BIT method is also not reliable. E.g. on x86_64 you are supposed
> to get the 32 bit ABI when calling through INT80 instead of syscall/sysenter,
> independent of the value of TIF_32BIT.
>
> A better way to do this is to add a separate compat_ioctl() method that
> converts this for you.
>
> The patch below is an example for the canonical way to do this. Not tested!
>
> Signed-off-by: Arnd Bergmann<arnd@arndb.de>
> ---
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 897bff3..20f88ad 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2297,6 +2297,49 @@ out:
>   	return r;
>   }
>
> +#ifdef CONFIG_COMPAT
> +struct compat_kvm_dirty_log {
> +	__u32 slot;
> +	__u32 padding1;
> +	union {
> +		compat_uptr_t dirty_bitmap; /* one bit per page */
> +		__u64 padding2;
> +	};
> +};
> +
> +static long kvm_vm_compat_ioctl(struct file *filp,
> +			   unsigned int ioctl, unsigned long arg)
> +{
> +	struct kvm *kvm = filp->private_data;
> +	int r;
> +
> +	if (kvm->mm != current->mm)
> +		return -EIO;
> +	switch (ioctl) {
> +	case KVM_GET_DIRTY_LOG: {
> +		struct compat_kvm_dirty_log compat_log;
> +		struct kvm_dirty_log log;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&compat_log, (void __user *)arg, sizeof log))
> +			goto out;
> +		log.slot	 = compat_log.slot;
> +		log.padding1	 = compat_log.padding1;
> +		log.padding2	 = compat_log.padding2;
> +		log.dirty_bitmap = compat_ptr(compat_log.dirty_bitmap);
> +
> +		r = kvm_vm_ioctl_get_dirty_log(kvm,&log.log);
> +		if (r)
> +			goto out;
> +		break;
> +	default:
> +		r = kvm_vm_ioctl(filp, ioctl, arg);
> +	}
> +
> +	return r;
> +}
> +#endif
> +
>   static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>   {
>   	struct page *page[1];
> @@ -2331,7 +2374,7 @@ static int kvm_vm_mmap(struct file *file, struct vm_area_struct *vma)
>   static struct file_operations kvm_vm_fops = {
>   	.release        = kvm_vm_release,
>   	.unlocked_ioctl = kvm_vm_ioctl,
> -	.compat_ioctl   = kvm_vm_ioctl,
> +	.compat_ioctl   = kvm_vm_compat_ioctl,
>   	.mmap           = kvm_vm_mmap,
>   };
>    

This is a bit painful - I tried to avoid compat_ioctl.  Maybe it's 
better to have dirty_bitmap_virt, given no existing users are impacted.

-- 
error compiling committee.c: too many arguments to function