Re: [PATCH 3/3] xen/privcmd: add PRIVCMD_MMAPBATCH_V2 ioctl

[David Vrabel <david.vrabel@citrix.com>]

On 23/08/12 20:40, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 23, 2012 at 06:13:46PM +0100, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> PRIVCMD_MMAPBATCH_V2 extends PRIVCMD_MMAPBATCH with an additional
>> field for reporting the error code for every frame that could not be
>> mapped.  libxc prefers PRIVCMD_MMAPBATCH_V2 over PRIVCMD_MMAPBATCH.
[...]
>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
>> index f8c1b6d..4f97160 100644
>> --- a/drivers/xen/privcmd.c
>> +++ b/drivers/xen/privcmd.c
>> @@ -275,34 +276,58 @@ static int mmap_return_errors(void *data, void *state)
>>  {
>>  	xen_pfn_t *mfnp = data;
>>  	struct mmap_batch_state *st = state;
>> +	int ret = 0;
>>  
>> -	return put_user(*mfnp, st->user++);
>> +	if (st->user_err) {
>> +		if ((*mfnp & 0xf0000000U) == 0xf0000000U)
>> +			ret = -ENOENT;
>> +		else if ((*mfnp & 0xf0000000U) == 0x80000000U)
>> +			ret = -EINVAL;
> 
> Yikes. Any way those 0xf000.. and 0x8000 can at least be #defined

Agreed.

>> +		else
>> +			ret = 0;
>> +		return __put_user(ret, st->user_err);
>> +	} else
>> +		return __put_user(*mfnp, st->user_mfn++);
>>  }
>>  
>>  static struct vm_operations_struct privcmd_vm_ops;
>>  
>> -static long privcmd_ioctl_mmap_batch(void __user *udata)
>> +static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
>>  {
>>  	int ret;
>> -	struct privcmd_mmapbatch m;
>> +	struct privcmd_mmapbatch_v2 m;
>>  	struct mm_struct *mm = current->mm;
>>  	struct vm_area_struct *vma;
>>  	unsigned long nr_pages;
>>  	LIST_HEAD(pagelist);
>>  	struct mmap_batch_state state;
>>  
>> +	printk("%s(%d)\n", __func__, version);
>> +
> 
> Hehe.

Heh.  I didn't polish up these patches as I wasn't sure this new
interface was useful.

>>  	if (!xen_initial_domain())
>>  		return -EPERM;
>>  
>> -	if (copy_from_user(&m, udata, sizeof(m)))
>> -		return -EFAULT;
>> +	if (version == 1) {
>> +		if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch)))
>> +			return -EFAULT;
>> +		if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
>> +			return -EFAULT;
> 
> That is new..

We use access_ok() here so we can use the less expensive __get_user()
and __put_user() later on.

>> +		if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
>> +			return -EFAULT;
>> +		if (!access_ok(VERIFY_READ, m.arr, m.num * sizeof(*m.arr)))
>> +			return -EFAULT;
>> +		if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
>> +			return -EFAULT;
> 
> I think the VERIFY_WRITE can cover both versions?

Yes, VERIFY_WRITE is a superset of VERIFY_READ.  In v1, m.arr is
read/write but in v2, m.arr is const so we use VERIFY_READ so the
userspace buffer can reside in a read-only section.

>> --- a/include/xen/privcmd.h
>> +++ b/include/xen/privcmd.h
>> @@ -62,6 +62,14 @@ struct privcmd_mmapbatch {
>>  	xen_pfn_t __user *arr; /* array of mfns - top nibble set on err */
>>  };
>>  
>> +struct privcmd_mmapbatch_v2 {
>> +	unsigned int num; /* number of pages to populate */
> 
> unsigend int? Not 'u32'?
>> +	domid_t dom;      /* target domain */
>> +	__u64 addr;       /* virtual address */
>> +	const xen_pfn_t __user *arr; /* array of mfns */
>> +	int __user *err;  /* array of error codes */
> 
> int? Not a specific type?

It's an existing interface supported by classic Xen kernels and
currently being used by libxc.  So while I agree that it's not the best
interface, I don't think it can be changed.

David