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

From: Jan Beulich
Date: Fri Jun 15 2018 - 10:35:10 EST


>>> 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?

> +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?

> + 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?

> +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.

> --- 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?

Jan