| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20161115045021.GA15992@danjae.aot.lge.com>
Date: Tue, 15 Nov 2016 13:50:21 +0900
From: Namhyung Kim <namhyung@...nel.org>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: virtio-dev@...ts.oasis-open.org, kvm@...r.kernel.org,
qemu-devel@...gnu.org, virtualization@...ts.linux-foundation.org,
LKML <linux-kernel@...r.kernel.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Radim Krčmář <rkrcmar@...hat.com>,
Anthony Liguori <aliguori@...zon.com>,
Anton Vorontsov <anton@...msg.org>,
Colin Cross <ccross@...roid.com>,
Kees Cook <keescook@...omium.org>,
Tony Luck <tony.luck@...el.com>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...nel.org>,
Minchan Kim <minchan@...nel.org>
Subject: Re: [PATCH 1/3] virtio: Basic implementation of virtio pstore driver
Hi Michael,
On Thu, Nov 10, 2016 at 06:39:55PM +0200, Michael S. Tsirkin wrote:
> On Sat, Aug 20, 2016 at 05:07:42PM +0900, Namhyung Kim wrote:
> > The virtio pstore driver provides interface to the pstore subsystem so
> > that the guest kernel's log/dump message can be saved on the host
> > machine. Users can access the log file directly on the host, or on the
> > guest at the next boot using pstore filesystem. It currently deals with
> > kernel log (printk) buffer only, but we can extend it to have other
> > information (like ftrace dump) later.
> >
> > It supports legacy PCI device using single order-2 page buffer.
>
> Do you mean a legacy virtio device? I don't see why
> you would want to support pre-1.0 mode.
> If you drop that, you can drop all cpu_to_virtio things
> and just use __le accessors.
I was thinking about the kvmtools which lacks 1.0 support AFAIK. But
I think it'd be better to always use __le type anyway. Will change.
>
> > It uses
> > two virtqueues - one for (sync) read and another for (async) write.
> > Since it cannot wait for write finished, it supports up to 128
> > concurrent IO. The buffer size is configurable now.
> >
> > Cc: Paolo Bonzini <pbonzini@...hat.com>
> > Cc: Radim Krčmář <rkrcmar@...hat.com>
> > Cc: "Michael S. Tsirkin" <mst@...hat.com>
> > Cc: Anthony Liguori <aliguori@...zon.com>
> > Cc: Anton Vorontsov <anton@...msg.org>
> > Cc: Colin Cross <ccross@...roid.com>
> > Cc: Kees Cook <keescook@...omium.org>
> > Cc: Tony Luck <tony.luck@...el.com>
> > Cc: Steven Rostedt <rostedt@...dmis.org>
> > Cc: Ingo Molnar <mingo@...nel.org>
> > Cc: Minchan Kim <minchan@...nel.org>
> > Cc: kvm@...r.kernel.org
> > Cc: qemu-devel@...gnu.org
> > Cc: virtualization@...ts.linux-foundation.org
> > Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> > ---
> > drivers/virtio/Kconfig | 10 +
> > drivers/virtio/Makefile | 1 +
> > drivers/virtio/virtio_pstore.c | 417 +++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/Kbuild | 1 +
> > include/uapi/linux/virtio_ids.h | 1 +
> > include/uapi/linux/virtio_pstore.h | 74 +++++++
> > 6 files changed, 504 insertions(+)
> > create mode 100644 drivers/virtio/virtio_pstore.c
> > create mode 100644 include/uapi/linux/virtio_pstore.h
> >
> > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> > index 77590320d44c..8f0e6c796c12 100644
> > --- a/drivers/virtio/Kconfig
> > +++ b/drivers/virtio/Kconfig
> > @@ -58,6 +58,16 @@ config VIRTIO_INPUT
> >
> > If unsure, say M.
> >
> > +config VIRTIO_PSTORE
> > + tristate "Virtio pstore driver"
> > + depends on VIRTIO
> > + depends on PSTORE
> > + ---help---
> > + This driver supports virtio pstore devices to save/restore
> > + panic and oops messages on the host.
> > +
> > + If unsure, say M.
> > +
> > config VIRTIO_MMIO
> > tristate "Platform bus driver for memory mapped virtio devices"
> > depends on HAS_IOMEM && HAS_DMA
> > diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> > index 41e30e3dc842..bee68cb26d48 100644
> > --- a/drivers/virtio/Makefile
> > +++ b/drivers/virtio/Makefile
> > @@ -5,3 +5,4 @@ virtio_pci-y := virtio_pci_modern.o virtio_pci_common.o
> > virtio_pci-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> > obj-$(CONFIG_VIRTIO_BALLOON) += virtio_balloon.o
> > obj-$(CONFIG_VIRTIO_INPUT) += virtio_input.o
> > +obj-$(CONFIG_VIRTIO_PSTORE) += virtio_pstore.o
> > diff --git a/drivers/virtio/virtio_pstore.c b/drivers/virtio/virtio_pstore.c
> > new file mode 100644
> > index 000000000000..0a63c7db4278
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_pstore.c
> > @@ -0,0 +1,417 @@
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pstore.h>
> > +#include <linux/virtio.h>
> > +#include <linux/virtio_config.h>
> > +#include <uapi/linux/virtio_ids.h>
> > +#include <uapi/linux/virtio_pstore.h>
> > +
> > +#define VIRT_PSTORE_ORDER 2
> > +#define VIRT_PSTORE_BUFSIZE (4096 << VIRT_PSTORE_ORDER)
> > +#define VIRT_PSTORE_NR_REQ 128
> > +
> > +struct virtio_pstore {
> > + struct virtio_device *vdev;
> > + struct virtqueue *vq[2];
>
> I'd add named fields instead of an array here, vq[0]
> vq[1] all over the place is hard to read.
Will change.
>
> > + struct pstore_info pstore;
> > + struct virtio_pstore_req req[VIRT_PSTORE_NR_REQ];
> > + struct virtio_pstore_res res[VIRT_PSTORE_NR_REQ];
> > + unsigned int req_id;
> > +
> > + /* Waiting for host to ack */
> > + wait_queue_head_t acked;
> > + int failed;
> > +};
> > +
> > +#define TYPE_TABLE_ENTRY(_entry) \
> > + { PSTORE_TYPE_##_entry, VIRTIO_PSTORE_TYPE_##_entry }
> > +
> > +struct type_table {
> > + int pstore;
> > + u16 virtio;
> > +} type_table[] = {
> > + TYPE_TABLE_ENTRY(DMESG),
> > +};
> > +
> > +#undef TYPE_TABLE_ENTRY
>
> let's avoid macros for now pls. In fact, I would just open-code this
> in to_virtio_type below. We can always change our minds later if
> lots of types are added.
Yep.
>
> > +
> > +
>
> single emoty line pls
Ok.
>
> > +static u16 to_virtio_type(struct virtio_pstore *vps, enum pstore_type_id type)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> > + if (type == type_table[i].pstore)
> > + return cpu_to_virtio16(vps->vdev, type_table[i].virtio);
> > + }
> > +
> > + return cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_TYPE_UNKNOWN);
>
> This assigns u16 to __virtio type, sparse will warn
> if you enable endian-ness checks.
> Pls fix that and generally, please make sure this is
> clean from sparse warnings.
I'll run sparse before sending patch next time.
>
> > +}
> > +
> > +static enum pstore_type_id from_virtio_type(struct virtio_pstore *vps, u16 type)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(type_table); i++) {
> > + if (virtio16_to_cpu(vps->vdev, type) == type_table[i].virtio)
> > + return type_table[i].pstore;
> > + }
> > +
> > + return PSTORE_TYPE_UNKNOWN;
> > +}
> > +
> > +static void virtpstore_ack(struct virtqueue *vq)
> > +{
> > + struct virtio_pstore *vps = vq->vdev->priv;
> > +
> > + wake_up(&vps->acked);
> > +}
> > +
> > +static void virtpstore_check(struct virtqueue *vq)
> > +{
> > + struct virtio_pstore *vps = vq->vdev->priv;
> > + struct virtio_pstore_res *res;
> > + unsigned int len;
> > +
> > + res = virtqueue_get_buf(vq, &len);
> > + if (res == NULL)
> > + return;
> > +
> > + if (virtio32_to_cpu(vq->vdev, res->ret) < 0)
> > + vps->failed = 1;
> > +}
> > +
> > +static void virt_pstore_get_reqs(struct virtio_pstore *vps,
> > + struct virtio_pstore_req **preq,
> > + struct virtio_pstore_res **pres)
> > +{
> > + unsigned int idx = vps->req_id++ % VIRT_PSTORE_NR_REQ;
> > +
> > + *preq = &vps->req[idx];
> > + *pres = &vps->res[idx];
> > +
> > + memset(*preq, 0, sizeof(**preq));
> > + memset(*pres, 0, sizeof(**pres));
> > +}
> > +
> > +static int virt_pstore_open(struct pstore_info *psi)
> > +{
> > + struct virtio_pstore *vps = psi->data;
> > + struct virtio_pstore_req *req;
> > + struct virtio_pstore_res *res;
> > + struct scatterlist sgo[1], sgi[1];
> > + struct scatterlist *sgs[2] = { sgo, sgi };
> > + unsigned int len;
> > +
> > + virt_pstore_get_reqs(vps, &req, &res);
> > +
> > + req->cmd = cpu_to_virtio16(vps->vdev, VIRTIO_PSTORE_CMD_OPEN);
> > +
> > + sg_init_one(sgo, req, sizeof(*req));
> > + sg_init_one(sgi, res, sizeof(*res));
> > + virtqueue_add_sgs(vps->vq[0], sgs, 1, 1, vps, GFP_KERNEL);
> > + virtqueue_kick(vps->vq[0]);
> > +
> > + wait_event(vps->acked, virtqueue_get_buf(vps->vq[0], &len));
>
> Does this block userspace in an uninterruptible wait if
> hardware is slow? That's not nice.
Yes, but it's not a common operation and I just wanted to make it
simple.
>
> > + return virtio32_to_cpu(vps->vdev, res->ret);
> > +}
> > +
[SNIP]
> > +struct virtio_pstore_fileinfo {
> > + __virtio64 id;
> > + __virtio32 count;
> > + __virtio16 type;
> > + __virtio16 unused;
> > + __virtio32 flags;
> > + __virtio32 len;
> > + __virtio64 time_sec;
> > + __virtio32 time_nsec;
> > + __virtio32 reserved;
> > +};
> > +
> > +struct virtio_pstore_config {
> > + __virtio32 bufsize;
> > +};
> > +
>
> What exactly does each field mean? I'm especially
> interested in time fields - maintaining a consistent
> time between host and guest is not a simple problem.
These are required by pstore and will be used to create corresponding
files in the pstore filesystem. The time fields are for mtime and
ctime and, I think, it's just a hint for user and doesn't require
strict consistency.
Thanks for your review!
Namhyung
>
> > +#endif /* _LINUX_VIRTIO_PSTORE_H */
> > --
> > 2.9.3
Powered by blists - more mailing lists