From: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
To: "Catangiu, Adrian Costin" <acatan@amazon.com>
Cc: "bonzini@gnu.org" <bonzini@gnu.org>, "Graf \(AWS\),
Alexander" <graf@amazon.de>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
"ghammer@redhat.com" <ghammer@redhat.com>,
"oridgar@gmail.com" <oridgar@gmail.com>,
"corbet@lwn.net" <corbet@lwn.net>,
"Weiss, Radu" <raduweis@amazon.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"virtualization@lists.linux-foundation.org"
<virtualization@lists.linux-foundation.org>,
"mst@redhat.com" <mst@redhat.com>,
"MacCarthaigh, Colm" <colmmacc@amazon.com>,
"Singh, Balbir" <sblbir@amazon.com>,
"Woodhouse, David" <dwmw@amazon.co.uk>
Subject: Re: [PATCH] drivers/virt: vmgenid: add vm generation id driver
Date: Fri, 16 Oct 2020 17:14:29 +0200 [thread overview]
Message-ID: <20201016151429.GD1807983@kroah.com> (raw)
In-Reply-To: <788878CE-2578-4991-A5A6-669DCABAC2F2@amazon.com>
On Fri, Oct 16, 2020 at 02:33:15PM +0000, Catangiu, Adrian Costin wrote:
> +config VMGENID
> + tristate "Virtual Machine Generation ID driver"
> + depends on ACPI
> + default M
Unless this is required to boot a machine, this should be removed.
> + help
> + This is a Virtual Machine Generation ID driver which provides
> + a virtual machine unique identifier. The provided UUID can be
> + watched through the FS interface exposed by this driver, and
> + thus can provide notifications for VM snapshot or cloning events.
Where is the uuid exposed in the filesystem?
> + This enables applications and libraries that store or cache
> + sensitive information, to know that they need to regenerate it
> + after process memory has been exposed to potential copying.
Module name?
> +
> config FSL_HV_MANAGER
> tristate "Freescale hypervisor management driver"
> depends on FSL_SOC
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index fd33124..a1f8dcc 100644
> --- a/drivers/virt/Makefile
> +++ b/drivers/virt/Makefile
> @@ -4,4 +4,5 @@
> #
>
> obj-$(CONFIG_FSL_HV_MANAGER) += fsl_hypervisor.o
> +obj-$(CONFIG_VMGENID) += vmgenid.o
> obj-y += vboxguest/
> diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
> new file mode 100644
> index 0000000..d314c72
> --- /dev/null
> +++ b/drivers/virt/vmgenid.c
> @@ -0,0 +1,419 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtual Machine Generation ID driver
> + *
> + * Copyright (C) 2018 Red Hat Inc, Copyright (C) 2020 Amazon.com Inc
That's not how you write copyright lines, please fix up.
> + * All rights reserved.
> + * Authors:
> + * Adrian Catangiu <acatan@amazon.com>
> + * Or Idgar <oridgar@gmail.com>
> + * Gal Hammer <ghammer@redhat.com>
> + *
> + */
> +#include <linux/acpi.h>
> +#include <linux/cdev.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/poll.h>
> +#include <linux/uuid.h>
> +#include <linux/vmgenid.h>
> +
> +#define DEV_NAME "vmgenid"
> +ACPI_MODULE_NAME(DEV_NAME);
> +
> +struct dev_data {
> + struct cdev cdev;
> + dev_t dev_id;
> + unsigned long map_buf;
> +
> + void *uuid_iomap;
> + uuid_t uuid;
> + wait_queue_head_t read_wait;
> +
> + atomic_t watchers;
> + atomic_t outdated_watchers;
> + wait_queue_head_t outdated_wait;
> +};
> +
> +struct file_data {
> + struct dev_data *dev_data;
> + uuid_t acked_uuid;
> +};
> +
> +static bool vmgenid_uuid_matches(struct dev_data *priv, uuid_t *uuid)
> +{
> + return !memcmp(uuid, &priv->uuid, sizeof(uuid_t));
> +}
> +
> +static void vmgenid_put_outdated_watchers(struct dev_data *priv)
> +{
> + if (atomic_dec_and_test(&priv->outdated_watchers))
> + wake_up_interruptible(&priv->outdated_wait);
> +}
> +
> +static int vmgenid_open(struct inode *inode, struct file *file)
> +{
> + struct dev_data *priv =
> + container_of(inode->i_cdev, struct dev_data, cdev);
> + struct file_data *file_data =
> + kzalloc(sizeof(struct file_data), GFP_KERNEL);
> +
> + if (!file_data)
> + return -ENOMEM;
> +
> + file_data->acked_uuid = priv->uuid;
> + file_data->dev_data = priv;
> +
> + file->private_data = file_data;
> + atomic_inc(&priv->watchers);
> +
> + return 0;
> +}
> +
> +static int vmgenid_close(struct inode *inode, struct file *file)
> +{
> + struct file_data *file_data = (struct file_data *) file->private_data;
> + struct dev_data *priv = file_data->dev_data;
> +
> + if (!vmgenid_uuid_matches(priv, &file_data->acked_uuid))
> + vmgenid_put_outdated_watchers(priv);
> + atomic_dec(&priv->watchers);
> + kfree(file->private_data);
> +
> + return 0;
> +}
> +
> +static ssize_t
> +vmgenid_read(struct file *file, char __user *ubuf, size_t nbytes, loff_t *ppos)
> +{
> + struct file_data *file_data =
> + (struct file_data *) file->private_data;
> + struct dev_data *priv = file_data->dev_data;
> + ssize_t ret;
> +
> + if (nbytes == 0)
> + return 0;
> + /* disallow partial UUID reads */
> + if (nbytes < sizeof(uuid_t))
> + return -EINVAL;
> + nbytes = sizeof(uuid_t);
> +
> + if (vmgenid_uuid_matches(priv, &file_data->acked_uuid)) {
> + if (file->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> + ret = wait_event_interruptible(
> + priv->read_wait,
> + !vmgenid_uuid_matches(priv, &file_data->acked_uuid)
> + );
> + if (ret)
> + return ret;
> + }
> +
> + ret = copy_to_user(ubuf, &priv->uuid, nbytes);
> + if (ret)
> + return -EFAULT;
> +
> + return nbytes;
> +}
> +
> +static ssize_t vmgenid_write(struct file *file, const char __user *ubuf,
> + size_t count, loff_t *ppos)
> +{
> + struct file_data *file_data = (struct file_data *) file->private_data;
> + struct dev_data *priv = file_data->dev_data;
> + uuid_t ack_uuid;
> +
> + /* disallow partial UUID writes */
> + if (count != sizeof(uuid_t))
> + return -EINVAL;
> + if (copy_from_user(&ack_uuid, ubuf, count))
> + return -EFAULT;
> + /* wrong UUID acknowledged */
> + if (!vmgenid_uuid_matches(priv, &ack_uuid))
> + return -EINVAL;
> +
> + if (!vmgenid_uuid_matches(priv, &file_data->acked_uuid)) {
> + /* update local view of UUID */
> + file_data->acked_uuid = ack_uuid;
> + vmgenid_put_outdated_watchers(priv);
> + }
> +
> + return (ssize_t)count;
> +}
> +
> +static __poll_t
> +vmgenid_poll(struct file *file, poll_table *wait)
> +{
> + __poll_t mask = 0;
> + struct file_data *file_data =
> + (struct file_data *) file->private_data;
> + struct dev_data *priv = file_data->dev_data;
> +
> + if (!vmgenid_uuid_matches(priv, &file_data->acked_uuid))
> + return EPOLLIN | EPOLLRDNORM;
> +
> + poll_wait(file, &priv->read_wait, wait);
> +
> + if (!vmgenid_uuid_matches(priv, &file_data->acked_uuid))
> + mask = EPOLLIN | EPOLLRDNORM;
> +
> + return mask;
> +}
> +
> +static long vmgenid_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct file_data *file_data =
> + (struct file_data *) file->private_data;
> + struct dev_data *priv = file_data->dev_data;
> + struct timespec __user *timeout = (void *) arg;
> + struct timespec kts;
> + ktime_t until;
> + int ret;
> +
> + switch (cmd) {
> + case VMGENID_GET_OUTDATED_WATCHERS:
> + ret = atomic_read(&priv->outdated_watchers);
> + break;
> + case VMGENID_WAIT_WATCHERS:
> + if (timeout) {
> + ret = copy_from_user(&kts, timeout, sizeof(kts));
> + if (ret)
> + return -EFAULT;
> + until = timespec_to_ktime(kts);
No validation of structure fields?
And you are exposing a kernel structure to userspace?
I don't see this function in Linus's tree right now, are you sure this
builds?
> + } else {
> + until = KTIME_MAX;
> + }
> +
> + ret = wait_event_interruptible_hrtimeout(
> + priv->outdated_wait,
> + !atomic_read(&priv->outdated_watchers),
> + until
> + );
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + return ret;
> +}
> +
> +static vm_fault_t vmgenid_vm_fault(struct vm_fault *vmf)
> +{
> + struct page *page;
> + struct file_data *file_data =
> + (struct file_data *) vmf->vma->vm_private_data;
You do know you don't need to cast stuff like this, right?
Also run checkpatch.pl so maintainers are not grumpy and tell you to run
checkpatch.pl.
> + struct dev_data *priv = file_data->dev_data;
> +
> + if (priv->map_buf) {
> + page = virt_to_page(priv->map_buf);
> + get_page(page);
> + vmf->page = page;
> + }
> +
> + return 0;
> +}
> +
> +static const struct vm_operations_struct vmgenid_vm_ops = {
> + .fault = vmgenid_vm_fault,
> +};
> +
> +static int vmgenid_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> + if (vma->vm_pgoff != 0 || vma_pages(vma) > 1)
> + return -EINVAL;
> +
> + if ((vma->vm_flags & VM_WRITE) != 0)
> + return -EPERM;
> +
> + vma->vm_ops = &vmgenid_vm_ops;
> + vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
> + vma->vm_private_data = file->private_data;
> +
> + return 0;
> +}
> +
> +static const struct file_operations fops = {
> + .owner = THIS_MODULE,
> + .mmap = vmgenid_mmap,
> + .open = vmgenid_open,
> + .release = vmgenid_close,
> + .read = vmgenid_read,
> + .write = vmgenid_write,
> + .poll = vmgenid_poll,
> + .compat_ioctl = vmgenid_ioctl,
> + .unlocked_ioctl = vmgenid_ioctl,
> +};
> +
> +static int vmgenid_acpi_map(struct dev_data *priv, acpi_handle handle)
> +{
> + int i;
> + phys_addr_t phys_addr;
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + acpi_status status;
> + union acpi_object *pss;
> + union acpi_object *element;
> +
> + status = acpi_evaluate_object(handle, "ADDR", NULL, &buffer);
> + if (ACPI_FAILURE(status)) {
> + ACPI_EXCEPTION((AE_INFO, status, "Evaluating ADDR"));
> + return -ENODEV;
> + }
> + pss = buffer.pointer;
> + if (!pss || pss->type != ACPI_TYPE_PACKAGE || pss->package.count != 2)
> + return -EINVAL;
> +
> + phys_addr = 0;
> + for (i = 0; i < pss->package.count; i++) {
> + element = &(pss->package.elements[i]);
> + if (element->type != ACPI_TYPE_INTEGER)
> + return -EINVAL;
> + phys_addr |= element->integer.value << i * 32;
> + }
> +
> + priv->uuid_iomap = acpi_os_map_memory(phys_addr, sizeof(uuid_t));
> + if (!priv->uuid_iomap) {
> + pr_err("Could not map memory at 0x%llx, size %u",
> + phys_addr,
> + (u32)sizeof(uuid_t));
> + return -ENOMEM;
> + }
> +
> + memcpy_fromio(&priv->uuid, priv->uuid_iomap, sizeof(uuid_t));
> + memcpy((void *) priv->map_buf, &priv->uuid, sizeof(uuid_t));
> +
> + return 0;
> +}
> +
> +static int vmgenid_acpi_add(struct acpi_device *device)
> +{
> + int ret;
> + struct dev_data *priv;
> +
> + if (!device)
> + return -EINVAL;
> +
> + priv = kzalloc(sizeof(struct dev_data), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->map_buf = get_zeroed_page(GFP_KERNEL);
> + if (!priv->map_buf) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> + device->driver_data = priv;
> +
> + init_waitqueue_head(&priv->read_wait);
> + atomic_set(&priv->watchers, 0);
> + atomic_set(&priv->outdated_watchers, 0);
> + init_waitqueue_head(&priv->outdated_wait);
> +
> + ret = vmgenid_acpi_map(priv, device->handle);
> + if (ret < 0)
> + goto err;
> +
> + ret = alloc_chrdev_region(&priv->dev_id, 0, 1, DEV_NAME);
Why is this not a misc device driver instead of messing around with all
of this "by hand"?
By doing things this way, you are not creating a device node in /dev/
automatically, right? How did you test this?
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2020, Amazon.com Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
License boilerplate is not needed.
> + */
> +
> +#ifndef _UAPI_LINUX_VMGENID_H
> +#define _UAPI_LINUX_VMGENID_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/time.h>
> +
> +#define VMGENID_IOCTL 0x2d
> +#define VMGENID_GET_OUTDATED_WATCHERS _IO(VMGENID_IOCTL, 1)
> +#define VMGENID_WAIT_WATCHERS _IOW(VMGENID_IOCTL, 2, struct timespec)
Why do you need ioctls for this? Why not just use read/write?
thanks,
greg k-h
next prev parent reply other threads:[~2020-10-16 15:15 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <AQHWo8lIfZnFKGe8nkGmhTCXwq5R3w==>
2020-10-16 14:33 ` [PATCH] drivers/virt: vmgenid: add vm generation id driver Catangiu, Adrian Costin
2020-10-16 15:00 ` Catangiu, Adrian Costin
2020-10-16 15:14 ` gregkh [this message]
2020-10-17 1:40 ` Jann Horn
2020-10-17 3:36 ` Willy Tarreau
2020-10-17 4:02 ` Jann Horn
2020-10-17 4:34 ` Colm MacCarthaigh
2020-10-17 5:01 ` Jann Horn
2020-10-17 5:29 ` Colm MacCarthaigh
2020-10-17 5:37 ` Willy Tarreau
2020-10-17 5:52 ` Jann Horn
2020-10-17 6:44 ` Willy Tarreau
2020-10-17 6:55 ` Jann Horn
2020-10-17 7:17 ` Willy Tarreau
2020-10-17 13:24 ` Jason A. Donenfeld
2020-10-17 18:06 ` Catangiu, Adrian Costin
2020-10-17 18:09 ` Alexander Graf
2020-10-18 2:08 ` Jann Horn
2020-10-20 9:35 ` Christian Borntraeger
2020-10-20 9:54 ` Alexander Graf
2020-10-20 16:54 ` Catangiu, Adrian Costin
2020-10-18 3:14 ` Colm MacCarthaigh
2020-10-18 15:52 ` Michael S. Tsirkin
2020-10-18 15:54 ` Andy Lutomirski
2020-10-18 15:59 ` Michael S. Tsirkin
2020-10-18 16:14 ` Andy Lutomirski
2020-10-19 15:00 ` Michael S. Tsirkin
2020-10-17 18:10 ` Andy Lutomirski
2020-10-19 17:15 ` Mathieu Desnoyers
2020-10-20 10:00 ` Alexander Graf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201016151429.GD1807983@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=acatan@amazon.com \
--cc=bonzini@gnu.org \
--cc=colmmacc@amazon.com \
--cc=corbet@lwn.net \
--cc=dwmw@amazon.co.uk \
--cc=ghammer@redhat.com \
--cc=graf@amazon.de \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=oridgar@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=raduweis@amazon.com \
--cc=sblbir@amazon.com \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).