On 15/06/18 16:35, Jan Beulich wrote:
>>>> On 15.06.18 at 15:17, <[email protected]> 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
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
