Re: [Xen-devel] [PATCH] xen: add new hypercall buffer mapping device

[Juergen Gross]

On 15/06/18 16:35, Jan Beulich wrote:
>>>> On 15.06.18 at 15:17, <jgross@xxxxxxxx> wrote:
>> --- /dev/null
>> +++ b/drivers/xen/privcmd-buf.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +
>> +/******************************************************************************
>> + * privcmd-buf.c
>> + *
>> + * Mmap of hypercall buffers.
>> + *
>> + * Copyright (c) 2018 Juergen Gross
>> + */
>> +
>> +#define pr_fmt(fmt) "xen:" KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/list.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/mm.h>
>> +#include <linux/slab.h>
>> +
>> +#include "privcmd.h"
>> +
>> +MODULE_LICENSE("GPL");
>> +
>> +static int limit = 64;
>> +module_param(limit, int, 0644);
> 
> Can this go negative? If not - "unsigned int" and "uint" prehaps?

Perhaps. ;-)

> 
>> +static int privcmd_buf_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +	struct privcmd_buf_private *file_priv = file->private_data;
>> +	struct privcmd_buf_vma_private *vma_priv;
>> +	unsigned int count = vma_pages(vma);
>> +	unsigned int i;
>> +	int ret = 0;
>> +
>> +	if (!(vma->vm_flags & VM_SHARED)) {
>> +		pr_err("Mapping must be shared\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (file_priv->allocated + count > limit) {
>> +		pr_err("Mapping limit reached!\n");
> 
> For both error messages - if you really want them, I think they should be
> made more helpful such that it is possible to identify the offender. Log at
> least process name and pid, or drop the messages?

I think dropping them should be fine.

> 
>> +		return -ENOSPC;
>> +	}
>> +
>> +	vma_priv = kzalloc(sizeof(*vma_priv) + count * sizeof(void *),
>> +			   GFP_KERNEL);
>> +	if (!vma_priv)
>> +		return -ENOMEM;
>> +
>> +	vma_priv->n_pages = count;
>> +	count = 0;
>> +	for (i = 0; i < vma_priv->n_pages; i++) {
>> +		vma_priv->pages[i] = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> +		if (!vma_priv->pages[i])
>> +			break;
>> +		count++;
>> +	}
>> +
>> +	mutex_lock(&file_priv->lock);
>> +
>> +	file_priv->allocated += count;
>> +
>> +	vma_priv->file_priv = file_priv;
>> +	vma_priv->users = 1;
>> +
>> +	vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
>> +	vma->vm_ops = &privcmd_buf_vm_ops;
>> +	vma->vm_private_data = vma_priv;
>> +
>> +	list_add(&vma_priv->list, &file_priv->list);
>> +
>> +	if (vma_priv->n_pages != count)
>> +		ret = -ENOMEM;
>> +	else
>> +		for (i = 0; i < vma_priv->n_pages; i++) {
>> +			ret = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
>> +					     vma_priv->pages[i]);
>> +			if (ret)
>> +				break;
>> +		}
>> +
>> +	if (ret)
>> +		privcmd_buf_vmapriv_free(vma_priv);
> 
> Don't you also need to undo the partially successful insertion?

No, this is done by generic mmap() handling when I'm returning an error.

> 
>> +struct miscdevice xen_privcmdbuf_dev = {
>> +	.minor = MISC_DYNAMIC_MINOR,
> 
> While dynamic minors are of course much better than fixed ones (as
> we used to use many years ago), but aren't they still a relatively
> limited resource? By setting a "mode" on a handle to the original
> privcmd interface, no new minor would be needed.

Hmm, I'm not very fond of this idea. That would make all privcmd
file ops rather clumsy. OTOH I can see the benfits.

Anyone wanting to comment on this idea?

> 
>> --- a/drivers/xen/privcmd.c
>> +++ b/drivers/xen/privcmd.c
>> @@ -1007,12 +1007,21 @@ static int __init privcmd_init(void)
>>  		pr_err("Could not register Xen privcmd device\n");
>>  		return err;
>>  	}
>> +
>> +	err = misc_register(&xen_privcmdbuf_dev);
>> +	if (err != 0) {
>> +		pr_err("Could not register Xen hypercall-buf device\n");
>> +		misc_deregister(&privcmd_dev);
>> +		return err;
> 
> Wouldn't this better be a warning only, without failing driver init?

No, I don't think so. We rather want the hypercall buffer handling to
be clean from now on.


Juergen