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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel