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

[Juergen Gross <jgross@xxxxxxxx>]

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

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