* Re: [PATCH] virtio-pci: fix use after free
From: Rusty Russell @ 2011-11-08 10:19 UTC (permalink / raw)
To: Amit Shah, Michael S. Tsirkin; +Cc: stable, linux-kernel, virtualization
In-Reply-To: <20111108054616.GC2068@amit-x200.redhat.com>
On Tue, 8 Nov 2011 11:16:16 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> On (Mon) 07 Nov 2011 [18:37:05], Michael S. Tsirkin wrote:
> > Commit 31a3ddda166cda86d2b5111e09ba4bda5239fae6 introduced
> > a use after free in virtio-pci. The main issue is
> > that the release method signals removal of the virtio device,
> > while remove signals removal of the pci device.
> >
> > For example, on driver removal or hot-unplug,
> > virtio_pci_release_dev is called before virtio_pci_remove.
> > We then might get a crash as virtio_pci_remove tries to use the
> > device freed by virtio_pci_release_dev.
> >
> > We allocate/free all resources together with the
> > pci device, so we can leave the release method empty.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Acked-by: Amit Shah <amit.shah@redhat.com>
>
> (note: Adding CC: stable@kernel.org to the commit log is the way
> patches get automatically pulled from upstream when committed; CC'ing
> stable on submissions won't help with that.)
Yeah, I fixed it though.
Cheers,
Rusty.
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Michael S. Tsirkin @ 2011-11-08 6:32 UTC (permalink / raw)
To: Rusty Russell
Cc: Anthony Liguori, virtualization, Sasha Levin, kvm, linux-kernel
In-Reply-To: <87aa87sd4y.fsf@rustcorp.com.au>
On Tue, Nov 08, 2011 at 10:23:33AM +1030, Rusty Russell wrote:
> Even so, should we just use the PCI cap list, and have each
> cap entry just contain a BIR & offset?
>
> Thanks,
> Rusty.
And size :)
I say, Rusty, did you see my patch? That's what it's doing,
I also addressed the issue that with KVM on x86, PIO is faster
than MMIO so we need to use it for notifications/isr.
--
MST
^ permalink raw reply
* Re: [PATCH] virtio-pci: fix use after free
From: Amit Shah @ 2011-11-08 5:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: stable, linux-kernel, virtualization
In-Reply-To: <20111107163703.GA10358@redhat.com>
On (Mon) 07 Nov 2011 [18:37:05], Michael S. Tsirkin wrote:
> Commit 31a3ddda166cda86d2b5111e09ba4bda5239fae6 introduced
> a use after free in virtio-pci. The main issue is
> that the release method signals removal of the virtio device,
> while remove signals removal of the pci device.
>
> For example, on driver removal or hot-unplug,
> virtio_pci_release_dev is called before virtio_pci_remove.
> We then might get a crash as virtio_pci_remove tries to use the
> device freed by virtio_pci_release_dev.
>
> We allocate/free all resources together with the
> pci device, so we can leave the release method empty.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Amit Shah <amit.shah@redhat.com>
(note: Adding CC: stable@kernel.org to the commit log is the way
patches get automatically pulled from upstream when committed; CC'ing
stable on submissions won't help with that.)
Amit
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Rusty Russell @ 2011-11-07 23:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Anthony Liguori, virtualization, Sasha Levin, kvm, linux-kernel
In-Reply-To: <20111107211413.GA11577@redhat.com>
On Mon, 7 Nov 2011 23:14:14 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Nov 07, 2011 at 03:46:23PM +1030, Rusty Russell wrote:
> > So far, the only three things make sense to have in a capability list:
> > MSI-X, the upper 32 feature bits, and the per-device config.
>
> You mean the queue # to MSI-X vector mapping?
Yep.
> One thing to remember is that it must be in the same type of BAR as
> the queue selection, since by PCI rules MMIO writes aren't I think
> ordered with PIO writes (it doesn't matter with KVM but might
> with another hypervisor).
OK, I'm slowly getting up to speed.
Next dumb q: Sasha, why did you introduce the idea of a separate
virtio-pci capability list, rather than just using PCI capabilities
directly? ie. instead of VIRTIO_PCI_C_LAYOUT, have VIRTIO_PCI_CORE,
VIRTIO_PCI_MSIX, VIRTIO_PCI_DEV_SPECIFIC?
Is it because we really want this stuff outside the PCI configuration
space? Even so, should we just use the PCI cap list, and have each
cap entry just contain a BIR & offset?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH] virtio-pci: fix use after free
From: Rusty Russell @ 2011-11-07 23:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: stable
In-Reply-To: <20111107163703.GA10358@redhat.com>
On Mon, 7 Nov 2011 18:37:05 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Commit 31a3ddda166cda86d2b5111e09ba4bda5239fae6 introduced
> a use after free in virtio-pci. The main issue is
> that the release method signals removal of the virtio device,
> while remove signals removal of the pci device.
>
> For example, on driver removal or hot-unplug,
> virtio_pci_release_dev is called before virtio_pci_remove.
> We then might get a crash as virtio_pci_remove tries to use the
> device freed by virtio_pci_release_dev.
>
> We allocate/free all resources together with the
> pci device, so we can leave the release method empty.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied, thanks. Will push once it's spent a day in linux-next.
Thanks,
Rusty.
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Michael S. Tsirkin @ 2011-11-07 21:14 UTC (permalink / raw)
To: Rusty Russell
Cc: Anthony Liguori, virtualization, Sasha Levin, kvm, linux-kernel
In-Reply-To: <871utktsuw.fsf@rustcorp.com.au>
On Mon, Nov 07, 2011 at 03:46:23PM +1030, Rusty Russell wrote:
> On Sun, 6 Nov 2011 23:38:49 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Sun, Nov 06, 2011 at 10:24:57PM +0200, Sasha Levin wrote:
> > > On Sun, 2011-11-06 at 09:30 +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Nov 04, 2011 at 04:53:05PM +0200, Sasha Levin wrote:
> > > > > > I guess my assumption is that most options will be in use,
> > > > > > not discarded dead-ends.
> > > > >
> > > > > I don't know about that. 64 bit features would be pretty rare for now -
> > > > > and I don't think that setting the alignment will be also enabled by
> > > > > default.
>
> The truth is somewhere between. New features tend to be used, and
> importantly, they have to be offered.
>
> Anyway, the *per-device* config part will be contiguous. So we're
> talking about pci-specific features.
>
> So far, the only three things make sense to have in a capability list:
> MSI-X, the upper 32 feature bits, and the per-device config.
>
> Thanks,
> Rusty.
You mean the queue # to MSI-X vector mapping?
One thing to remember is that it must be in the same type of BAR as
the queue selection, since by PCI rules MMIO writes aren't I think
ordered with PIO writes (it doesn't matter with KVM but might
with another hypervisor).
--
MST
^ permalink raw reply
* Re: Regression: patch " hvc_console: display printk messages on console." causing infinite loop with 3.2-rc0 + Xen.
From: Greg KH @ 2011-11-07 20:24 UTC (permalink / raw)
To: Stephen Rothwell
Cc: xen-devel, Konrad Rzeszutek Wilk, Benjamin Herrenschmidt, ppc-dev,
miche, linux-kernel, virtualization, Anton Blanchard, Amit Shah,
Linus
In-Reply-To: <20111107171942.fe21429583491475f245aa08@canb.auug.org.au>
On Mon, Nov 07, 2011 at 05:19:42PM +1100, Stephen Rothwell wrote:
> Hi Greg,
>
> On Wed, 2 Nov 2011 18:30:12 -0700 Greg KH <gregkh@suse.de> wrote:
> >
> > On Wed, Nov 02, 2011 at 12:13:09PM +1100, Stephen Rothwell wrote:
> > >
> > > On Thu, 27 Oct 2011 07:48:06 +0200 Greg KH <gregkh@suse.de> wrote:
> > > >
> > > > On Thu, Oct 27, 2011 at 01:30:08AM -0400, Konrad Rzeszutek Wilk wrote:
> > > > > Hey Miche.
> > > > >
> > > > > The git commit 361162459f62dc0826b82c9690a741a940f457f0:
> > > > >
> > > > > hvc_console: display printk messages on console.
> > > > >
> > > > > is causing an infinite loop when booting Linux under Xen, as so:
> > > >
> > > > Ick, not good, thanks for letting us know.
> > >
> > > Indeed. I am wondering why it was put in a tree and sent to Linus without
> > > any Acks or even being replied to by anyone. It appeared in the tty tree
> > > between Oct 14 and Oct 25 (while I was unfortunately on vacation). If
> > > anyone had tried to boot this on any PowerPC server, it would have been
> > > immediately obvious (as it was when I booted Linus' tree last night).
> > >
> > > And the original author expressed doubts as to his understanding of how
> > > it should all work anyway.
> > >
> > > Just a little more care, please.
> > >
> > > I would vote for reverting the original and having it resubmitted with
> > > corrections at some later date.
> >
> > You are right, I will go do that, sorry for the problems.
>
> Ping ...
>
> Linus can you please just revert 361162459f62dc0826b82c9690a741a940f457f0
> "hvc_console: display printk messages on console" as it breaks consoles
> for all PowerPC server machines.
Thanks for doing this, I was going to include it in my next pull request
after 3.2-rc1 was out, but you are right, it should have gone in sooner.
greg k-h
^ permalink raw reply
* [PATCH] virtio-pci: fix use after free
From: Michael S. Tsirkin @ 2011-11-07 16:37 UTC (permalink / raw)
To: Rusty Russell, Michael S. Tsirkin, virtualization, linux-kernel; +Cc: stable
Commit 31a3ddda166cda86d2b5111e09ba4bda5239fae6 introduced
a use after free in virtio-pci. The main issue is
that the release method signals removal of the virtio device,
while remove signals removal of the pci device.
For example, on driver removal or hot-unplug,
virtio_pci_release_dev is called before virtio_pci_remove.
We then might get a crash as virtio_pci_remove tries to use the
device freed by virtio_pci_release_dev.
We allocate/free all resources together with the
pci device, so we can leave the release method empty.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_pci.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index d4ed130..a989b41 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -730,11 +730,11 @@ static struct virtio_config_ops virtio_pci_config_ops = {
static void virtio_pci_release_dev(struct device *_d)
{
- struct virtio_device *dev = container_of(_d, struct virtio_device,
- dev);
- struct virtio_pci_device *vp_dev = to_vp_device(dev);
-
- kfree(vp_dev);
+ /*
+ * No need for a release method as we allocate/free
+ * all devices together with the pci devices.
+ * Provide an empty one to avoid getting a warning from core.
+ */
}
/* the PCI probing function */
@@ -822,6 +822,7 @@ static void __devexit virtio_pci_remove(struct pci_dev *pci_dev)
virtio_pci_iounmap(vp_dev);
pci_release_regions(pci_dev);
pci_disable_device(pci_dev);
+ kfree(vp_dev);
}
#ifdef CONFIG_PM
--
1.7.5.53.gc233e
^ permalink raw reply related
* Re: Regression: patch " hvc_console: display printk messages on console." causing infinite loop with 3.2-rc0 + Xen.
From: Stephen Rothwell @ 2011-11-07 6:19 UTC (permalink / raw)
To: Linus
Cc: xen-devel, Konrad Rzeszutek Wilk, Benjamin Herrenschmidt, miche,
linux-kernel, virtualization, Anton Blanchard, Amit Shah, ppc-dev,
Greg KH
In-Reply-To: <20111103013012.GB3449@suse.de>
[-- Attachment #1.1: Type: text/plain, Size: 1664 bytes --]
Hi Greg,
On Wed, 2 Nov 2011 18:30:12 -0700 Greg KH <gregkh@suse.de> wrote:
>
> On Wed, Nov 02, 2011 at 12:13:09PM +1100, Stephen Rothwell wrote:
> >
> > On Thu, 27 Oct 2011 07:48:06 +0200 Greg KH <gregkh@suse.de> wrote:
> > >
> > > On Thu, Oct 27, 2011 at 01:30:08AM -0400, Konrad Rzeszutek Wilk wrote:
> > > > Hey Miche.
> > > >
> > > > The git commit 361162459f62dc0826b82c9690a741a940f457f0:
> > > >
> > > > hvc_console: display printk messages on console.
> > > >
> > > > is causing an infinite loop when booting Linux under Xen, as so:
> > >
> > > Ick, not good, thanks for letting us know.
> >
> > Indeed. I am wondering why it was put in a tree and sent to Linus without
> > any Acks or even being replied to by anyone. It appeared in the tty tree
> > between Oct 14 and Oct 25 (while I was unfortunately on vacation). If
> > anyone had tried to boot this on any PowerPC server, it would have been
> > immediately obvious (as it was when I booted Linus' tree last night).
> >
> > And the original author expressed doubts as to his understanding of how
> > it should all work anyway.
> >
> > Just a little more care, please.
> >
> > I would vote for reverting the original and having it resubmitted with
> > corrections at some later date.
>
> You are right, I will go do that, sorry for the problems.
Ping ...
Linus can you please just revert 361162459f62dc0826b82c9690a741a940f457f0
"hvc_console: display printk messages on console" as it breaks consoles
for all PowerPC server machines.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Dmitry Torokhov @ 2011-11-07 5:51 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, joe@perches.com, jkosina@suse.cz
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481AA68580@TK5EX14MBXC128.redmond.corp.microsoft.com>
Hi K. Y,
On Mon, Nov 07, 2011 at 01:04:53AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> > Sent: Saturday, November 05, 2011 2:48 AM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> > joe@perches.com; jkosina@suse.cz
> > Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
> >
> > Hi KY,
>
> Dimitry,
>
> Let me begin by thanking you for taking the time to review. I have incorporated
> pretty much all your suggestions.
Thank you very much for considering my suggestions.
> >
> > Instead of potentially ever-increasing buffer that you also allocate
> > (and it looks like leaking on every callback invocation) can you just
> > repeat the read if you know that there are more data and use single
> > pre-allocated buffer?
>
> The ring-buffer protocol is such that we need to consume the full message.
> Also, why do you say we are leaking memory?
Ah, OK, I see, we keep reading until read returns 0-sized reply and then
we free the buffer... Never mind then.
> > > +
> > > + hid_dev->ll_driver = &mousevsc_ll_driver;
> > > + hid_dev->driver = &mousevsc_hid_driver;
> >
> > You are not really hid driver; you are more of a "provider" so why do
> > you need to set hid_dev->driver in addition to hid_dev->ll_driver?
> >
> True, but hid_parse_report() expects that the driver field be set; so I
> need to fake this.
If you supply .parse() method for your mousevsc_ll_driver structure and
call hid_parse_report() from it then HID core will set the default
driver and call parse at appropriate time.
>
> > > + hid_dev->bus = BUS_VIRTUAL;
> > > + hid_dev->vendor = input_dev->hid_dev_info.vendor;
> > > + hid_dev->product = input_dev->hid_dev_info.product;
> > > + hid_dev->version = input_dev->hid_dev_info.version;
> > > + input_dev->hid_device = hid_dev;
> > > +
> > > + sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
> >
> > strlcpy?
> >
> > > +
> > > + ret = hid_parse_report(hid_dev, input_dev->report_desc,
> > > + input_dev->report_desc_size);
> > > +
> > > + if (ret) {
> > > + hid_err(hid_dev, "parse failed\n");
> > > + goto probe_err1;
> > > + }
> > > +
> > > + ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT |
> > HID_CONNECT_HIDDEV);
> >
> > Why do you need to call hid_hw_start instead of letting HID core figure
> > it out for you?
>
> I am not a hid expert; but all hid low level drivers appear to do this.
> Initially, I was directly invoking hid_connect() directly and based on your
> Input, I chose to use hid_hw_start() which all other drivers are using.
Note that the users of hid_hw_start() actually are not low level
drivers, such as usbhid or bluetooth hidp, but higher-level drivers,
such as hid-wacom, hid-a4tech, etc. Since your driver is a low-level
driver (a provider so to speak) it should not call hid_hw_start() on its
own but rather wait for the hid code to do it.
Still, I am not a HID expert either so I'll defer to Jiri here.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Rusty Russell @ 2011-11-07 5:16 UTC (permalink / raw)
To: Michael S. Tsirkin, Sasha Levin
Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <20111106213849.GA14292@redhat.com>
On Sun, 6 Nov 2011 23:38:49 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Sun, Nov 06, 2011 at 10:24:57PM +0200, Sasha Levin wrote:
> > On Sun, 2011-11-06 at 09:30 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Nov 04, 2011 at 04:53:05PM +0200, Sasha Levin wrote:
> > > > > I guess my assumption is that most options will be in use,
> > > > > not discarded dead-ends.
> > > >
> > > > I don't know about that. 64 bit features would be pretty rare for now -
> > > > and I don't think that setting the alignment will be also enabled by
> > > > default.
The truth is somewhere between. New features tend to be used, and
importantly, they have to be offered.
Anyway, the *per-device* config part will be contiguous. So we're
talking about pci-specific features.
So far, the only three things make sense to have in a capability list:
MSI-X, the upper 32 feature bits, and the per-device config.
Thanks,
Rusty.
^ permalink raw reply
* RE: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: KY Srinivasan @ 2011-11-07 1:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
ohering@suse.com, joe@perches.com, jkosina@suse.cz
In-Reply-To: <20111105064739.GA31111@core.coreip.homeip.net>
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
> Sent: Saturday, November 05, 2011 2:48 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
> joe@perches.com; jkosina@suse.cz
> Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
>
> Hi KY,
Dimitry,
Let me begin by thanking you for taking the time to review. I have incorporated
pretty much all your suggestions. These changes have cleaned up the code
considerably. I will send the patch out that incorporates these changes shortly. I will
also send out the next version of the patch to move the mouse driver out of staging.
More details in-line.
> Can we call it mousevsc_alloc_device?
Done.
>
> > +{
> > + struct mousevsc_dev *input_dev;
> > +
> > + input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> > +
> > + if (!input_dev)
> > + return NULL;
> > +
> > + input_dev->device = device;
> > + hv_set_drvdata(device, input_dev);
> > + init_completion(&input_dev->wait_event);
> > +
> > + return input_dev;
> > +}
> > +
> > +static void free_input_device(struct mousevsc_dev *device)
>
> Can we call it mousevsc_free_device?
Done.
> This could probably be:
>
> static const mousevsc_prt_msg ack = {
> .type = PIPE_MESSAGE_DATA,
> .size = sizeof(struct synthhid_device_info_ack),
> .ack = {
> .header = {
> .type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> .size = 1,
> },
> .reserved = 0,
> },
> };
>
Done.
>
> > +
> > + /* Assume success for now */
> > + input_device->dev_info_status = 0;
> > +
> > + memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> > + sizeof(struct hv_input_dev_info));
> > +
>
> or simply:
>
> input_device->hid_dev_info = device_info->hid_dev_info;
Done.
>
> > + desc = &device_info->hid_descriptor;
> > + WARN_ON(desc->bLength == 0);
>
> Should also goto cleanup as such descriptor is not usable.
Done.
>
> > +
> > + input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> > +
> > + if (!input_device->hid_desc)
>
> if you do
>
> input_device->dev_info_status = -ENOMEM;
>
> you could use it later instead of defaulting to -ENOMEM.
>
> > + goto cleanup;
> > +
> > + memcpy(input_device->hid_desc, desc, desc->bLength);
> > +
> > + input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> > + if (input_device->report_desc_size == 0)
>
> input_device->dev_info_status = -EINVAL;
>
> > + goto cleanup;
> > + input_device->report_desc = kzalloc(input_device->report_desc_size,
> > + GFP_ATOMIC);
> > +
> > + if (!input_device->report_desc)
>
> input_device->dev_info_status = -ENOMEM;
>
> > + goto cleanup;
> > +
> > + memcpy(input_device->report_desc,
> > + ((unsigned char *)desc) + desc->bLength,
> > + desc->desc[0].wDescriptorLength);
> > +
> > + /* Send the ack */
> > + memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > + ack.type = PIPE_MESSAGE_DATA;
> > + ack.size = sizeof(struct synthhid_device_info_ack);
> > +
> > + ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> > + ack.ack.header.size = 1;
> > + ack.ack.reserved = 0;
> > +
> > + ret = vmbus_sendpacket(input_device->device->channel,
> > + &ack,
> > + sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> > + sizeof(struct synthhid_device_info_ack),
> > + (unsigned long)&ack,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > + if (ret != 0)
>
> input_device->dev_info_status = ret;
>
> > + goto cleanup;
> > +
> > + complete(&input_device->wait_event);
> > +
> > + return;
> > +
> > +cleanup:
> > + kfree(input_device->hid_desc);
> > + input_device->hid_desc = NULL;
> > +
> > + kfree(input_device->report_desc);
> > + input_device->report_desc = NULL;
>
> Do you have to clean it up here? You still need to clean it up in main
> unwind path so consolidate both error and success path and just signal
> completion and let main code figure it all out.
Done.
> > +
> > +static void mousevsc_on_channel_callback(void *context)
> > +{
> > + const int packet_size = 0x100;
> > + int ret;
> > + struct hv_device *device = context;
> > + u32 bytes_recvd;
> > + u64 req_id;
> > + struct vmpacket_descriptor *desc;
> > + unsigned char *buffer;
> > + int bufferlen = packet_size;
> > +
> > + buffer = kmalloc(bufferlen, GFP_ATOMIC);
> > + if (!buffer)
> > + return;
> > +
> > + do {
> > + ret = vmbus_recvpacket_raw(device->channel, buffer,
> > + bufferlen, &bytes_recvd, &req_id);
> > +
> > + switch (ret) {
> > + case 0:
> > + if (bytes_recvd <= 0) {
> > + kfree(buffer);
> > + return;
> > + }
> > + desc = (struct vmpacket_descriptor *)buffer;
> > +
> > + switch (desc->type) {
> > + case VM_PKT_COMP:
> > + break;
> > +
> > + case VM_PKT_DATA_INBAND:
> > + mousevsc_on_receive(device, desc);
> > + break;
> > +
> > + default:
> > + pr_err("unhandled packet type %d, tid %llx len
> %d\n",
> > + desc->type,
> > + req_id,
> > + bytes_recvd);
> > + break;
> > + }
> > +
> > + break;
> > +
> > + case -ENOBUFS:
> > + kfree(buffer);
> > + /* Handle large packet */
> > + bufferlen = bytes_recvd;
> > + buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
>
> Instead of potentially ever-increasing buffer that you also allocate
> (and it looks like leaking on every callback invocation) can you just
> repeat the read if you know that there are more data and use single
> pre-allocated buffer?
The ring-buffer protocol is such that we need to consume the full message.
Also, why do you say we are leaking memory?
>
> > +
> > + if (!buffer)
> > + return;
> > +
> > + break;
> > + }
> > + } while (1);
> > +
> > +}
> > +
> > +static int mousevsc_connect_to_vsp(struct hv_device *device)
> > +{
> > + int ret = 0;
> > + int t;
> > + struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> > + struct mousevsc_prt_msg *request;
> > + struct mousevsc_prt_msg *response;
> > +
> > + request = &input_dev->protocol_req;
> > +
> > + memset(request, 0, sizeof(struct mousevsc_prt_msg));
> > +
> > + request->type = PIPE_MESSAGE_DATA;
> > + request->size = sizeof(struct synthhid_protocol_request);
> > +
> > + request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> > + request->request.header.size = sizeof(unsigned int);
> > + request->request.version_requested.version =
> SYNTHHID_INPUT_VERSION;
> >
>
> This is constant data; just do the same as with ack in
> on_receive_device_info. It does not need to be a part of input_dev, does
> it?
Done.
>
> > + ret = vmbus_sendpacket(device->channel, request,
> > + sizeof(struct pipe_prt_msg) -
> > + sizeof(unsigned char) +
> > + sizeof(struct synthhid_protocol_request),
> > + (unsigned long)request,
> > + VM_PKT_DATA_INBAND,
> > +
> VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> > + if (ret)
> > + goto cleanup;
> > +
> > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> > + if (!t) {
> > + ret = -ETIMEDOUT;
> > + goto cleanup;
> > + }
> > +
> > + response = &input_dev->protocol_resp;
> > +
> > + if (!response->response.approved) {
> > + pr_err("synthhid protocol request failed (version %d)\n",
> > + SYNTHHID_INPUT_VERSION);
> > + ret = -ENODEV;
> > + goto cleanup;
> > + }
> > +
> > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
>
> 5 seconds? That's a long time of HV to respond...
Well, this is a safe number! We never wait this long.
>
> > + if (!t) {
> > + ret = -ETIMEDOUT;
> > + goto cleanup;
> > + }
> > +
> > + /*
> > + * We should have gotten the device attr, hid desc and report
> > + * desc at this point
> > + */
> > + if (input_dev->dev_info_status)
> > + ret = -ENOMEM;
>
> Just do
>
> ret = input_dev->dev_info_status;
Done.
>
> unconditionally.
>
> > +
> > +cleanup:
> > +
> > + return ret;
> > +}
> > +
> > +static int mousevsc_hid_open(struct hid_device *hid)
> > +{
> > + return 0;
> > +}
> > +
> > +static int mousevsc_hid_start(struct hid_device *hid)
> > +{
> > + return 0;
> > +}
> > +
> > +static void mousevsc_hid_close(struct hid_device *hid)
> > +{
> > +}
> > +
> > +static void mousevsc_hid_stop(struct hid_device *hid)
> > +{
> > +}
> > +
> > +static struct hid_ll_driver mousevsc_ll_driver = {
> > + .open = mousevsc_hid_open,
> > + .close = mousevsc_hid_close,
> > + .start = mousevsc_hid_start,
> > + .stop = mousevsc_hid_stop,
> > +};
> > +
> > +static struct hid_driver mousevsc_hid_driver;
> > +
> > +static int mousevsc_probe(struct hv_device *device,
> > + const struct hv_vmbus_device_id *dev_id)
> > +{
> > + int ret = 0;
> > + struct mousevsc_dev *input_dev;
> > + struct hid_device *hid_dev;
> > +
> > + input_dev = alloc_input_device(device);
> > +
> > + if (!input_dev)
> > + return -ENOMEM;
> > +
> > + input_dev->init_complete = false;
>
> Should this also be in alloc_input_device()?
Done.
>
> > +
> > + ret = vmbus_open(device->channel,
> > + INPUTVSC_SEND_RING_BUFFER_SIZE,
> > + INPUTVSC_RECV_RING_BUFFER_SIZE,
> > + NULL,
> > + 0,
> > + mousevsc_on_channel_callback,
> > + device
> > + );
> > +
>
> This blank line is extra IMO.
>
> > + if (ret != 0) {
> > + free_input_device(input_dev);
> > + return ret;
>
> Please do not mix error unwinding with gotos and unwinding in error
> handling branches.
Done.
>
> > + }
> > +
> > + ret = mousevsc_connect_to_vsp(device);
> > +
>
> This blank line is extra IMO.
>
> > + if (ret != 0)
> > + goto probe_err0;
> > +
> > + /* workaround SA-167 */
> > + if (input_dev->report_desc[14] == 0x25)
> > + input_dev->report_desc[14] = 0x29;
> > +
> > + hid_dev = hid_allocate_device();
> > + if (IS_ERR(hid_dev)) {
> > + ret = PTR_ERR(hid_dev);
> > + goto probe_err0;
> > + }
> > +
> > + hid_dev->ll_driver = &mousevsc_ll_driver;
> > + hid_dev->driver = &mousevsc_hid_driver;
>
> You are not really hid driver; you are more of a "provider" so why do
> you need to set hid_dev->driver in addition to hid_dev->ll_driver?
>
True, but hid_parse_report() expects that the driver field be set; so I
need to fake this.
> > + hid_dev->bus = BUS_VIRTUAL;
> > + hid_dev->vendor = input_dev->hid_dev_info.vendor;
> > + hid_dev->product = input_dev->hid_dev_info.product;
> > + hid_dev->version = input_dev->hid_dev_info.version;
> > + input_dev->hid_device = hid_dev;
> > +
> > + sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
>
> strlcpy?
>
> > +
> > + ret = hid_parse_report(hid_dev, input_dev->report_desc,
> > + input_dev->report_desc_size);
> > +
> > + if (ret) {
> > + hid_err(hid_dev, "parse failed\n");
> > + goto probe_err1;
> > + }
> > +
> > + ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT |
> HID_CONNECT_HIDDEV);
>
> Why do you need to call hid_hw_start instead of letting HID core figure
> it out for you?
I am not a hid expert; but all hid low level drivers appear to do this.
Initially, I was directly invoking hid_connect() directly and based on your
Input, I chose to use hid_hw_start() which all other drivers are using.
>
> > +
> > + if (ret) {
> > + hid_err(hid_dev, "hw start failed\n");
> > + goto probe_err1;
> > + }
> > +
> > + input_dev->connected = true;
> > + input_dev->init_complete = true;
> > +
> > + return ret;
> > +
> > +probe_err1:
> > + hid_destroy_device(hid_dev);
> > +
> > +probe_err0:
> > + vmbus_close(device->channel);
> > + free_input_device(input_dev);
> > + return ret;
> > +}
> > +
> > +
> > +static int mousevsc_remove(struct hv_device *dev)
> > +{
> > + struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> > +
> > + vmbus_close(dev->channel);
> > +
> > + if (input_dev->connected) {
>
> Can we even get here if input_dev->connected == false?
>
> > + hidinput_disconnect(input_dev->hid_device);
>
> You did not explicitly connect hidinput; leave disconnecting to the HID
> core as well.
>
> It looks like all that is needed is:
>
> static int mousevsc_remove(struct hv_device *dev)
> {
> struct mousevsc_dev *mouse = hv_get_drvdata(dev);
>
> vmbus_close(dev->channel);
> hid_destroy_device(mouse->hid_device);
> mousevcs_free_device(mouse);
>
> return 0;
> }
Done.
>
> > + input_dev->connected = false;
> > + hid_destroy_device(input_dev->hid_device);
> > + }
> > +
> > + free_input_device(input_dev);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct hv_vmbus_device_id id_table[] = {
> > + /* Mouse guid */
> > + { VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> > + 0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> > + { },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(vmbus, id_table);
> > +
> > +static struct hv_driver mousevsc_drv = {
> > + .name = KBUILD_MODNAME,
> > + .id_table = id_table,
> > + .probe = mousevsc_probe,
> > + .remove = mousevsc_remove,
> > +};
> > +
> > +static int __init mousevsc_init(void)
> > +{
> > + return vmbus_driver_register(&mousevsc_drv);
> > +}
> > +
> > +static void __exit mousevsc_exit(void)
> > +{
> > + vmbus_driver_unregister(&mousevsc_drv);
> > +}
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION(HV_DRV_VERSION);
> > +module_init(mousevsc_init);
> > +module_exit(mousevsc_exit);
> > --
> > 1.7.4.1
> >
>
> Thanks.
Thank you!
K. Y
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Michael S. Tsirkin @ 2011-11-06 21:38 UTC (permalink / raw)
To: Sasha Levin; +Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <1320611097.3299.10.camel@lappy>
On Sun, Nov 06, 2011 at 10:24:57PM +0200, Sasha Levin wrote:
> On Sun, 2011-11-06 at 09:30 +0200, Michael S. Tsirkin wrote:
> > On Fri, Nov 04, 2011 at 04:53:05PM +0200, Sasha Levin wrote:
> > > > > > > As you said, the PCI cap list was introduced both to save space (which
> > > > > > > is not the motivation here), and because it's a very efficient
> > > > > >
> > > > > > It's actually pretty inefficient - there's an overhead of 3 bytes for
> > > > > > each vendor specific option.
> > > > >
> > > > > It's efficient because while you pay a small price for each optional
> > > > > option it also means that that option is optional and won't clutter the
> > > > > config space if it's not really in use.
> > > >
> > > > I guess my assumption is that most options will be in use,
> > > > not discarded dead-ends.
> > >
> > > I don't know about that. 64 bit features would be pretty rare for now -
> > > and I don't think that setting the alignment will be also enabled by
> > > default.
> >
> > Setting the alignment might not be *used* by default but
> > I think it must be enabled by default to allow bios access.
> >
> > > I think that we're looking at it differently because I assume that any
> > > feature we add at this point would be optional and used only in specific
> > > scenarios, while you think that everything added will be used most of
> > > the time.
> >
> > Options must often be present even if not used. For example, as device
> > has no way to know whether a guest will want to program alignment, it
> > has to make that option available.
>
> They should be enabled, but heres the difference between the two
> approaches is that if it's cap it simply won't be there,
How can it not be there? They layout is specified by host,
not by guest.
> while in the
> other case it would just remain empty at some random offset of the
> struct.
>
> --
>
> Sasha.
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Sasha Levin @ 2011-11-06 20:24 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <20111106073007.GA7146@redhat.com>
On Sun, 2011-11-06 at 09:30 +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 04, 2011 at 04:53:05PM +0200, Sasha Levin wrote:
> > > > > > As you said, the PCI cap list was introduced both to save space (which
> > > > > > is not the motivation here), and because it's a very efficient
> > > > >
> > > > > It's actually pretty inefficient - there's an overhead of 3 bytes for
> > > > > each vendor specific option.
> > > >
> > > > It's efficient because while you pay a small price for each optional
> > > > option it also means that that option is optional and won't clutter the
> > > > config space if it's not really in use.
> > >
> > > I guess my assumption is that most options will be in use,
> > > not discarded dead-ends.
> >
> > I don't know about that. 64 bit features would be pretty rare for now -
> > and I don't think that setting the alignment will be also enabled by
> > default.
>
> Setting the alignment might not be *used* by default but
> I think it must be enabled by default to allow bios access.
>
> > I think that we're looking at it differently because I assume that any
> > feature we add at this point would be optional and used only in specific
> > scenarios, while you think that everything added will be used most of
> > the time.
>
> Options must often be present even if not used. For example, as device
> has no way to know whether a guest will want to program alignment, it
> has to make that option available.
They should be enabled, but heres the difference between the two
approaches is that if it's cap it simply won't be there, while in the
other case it would just remain empty at some random offset of the
struct.
--
Sasha.
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Michael S. Tsirkin @ 2011-11-06 7:30 UTC (permalink / raw)
To: Sasha Levin; +Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <1320418385.3334.25.camel@lappy>
On Fri, Nov 04, 2011 at 04:53:05PM +0200, Sasha Levin wrote:
> > > > > As you said, the PCI cap list was introduced both to save space (which
> > > > > is not the motivation here), and because it's a very efficient
> > > >
> > > > It's actually pretty inefficient - there's an overhead of 3 bytes for
> > > > each vendor specific option.
> > >
> > > It's efficient because while you pay a small price for each optional
> > > option it also means that that option is optional and won't clutter the
> > > config space if it's not really in use.
> >
> > I guess my assumption is that most options will be in use,
> > not discarded dead-ends.
>
> I don't know about that. 64 bit features would be pretty rare for now -
> and I don't think that setting the alignment will be also enabled by
> default.
Setting the alignment might not be *used* by default but
I think it must be enabled by default to allow bios access.
> I think that we're looking at it differently because I assume that any
> feature we add at this point would be optional and used only in specific
> scenarios, while you think that everything added will be used most of
> the time.
Options must often be present even if not used. For example, as device
has no way to know whether a guest will want to program alignment, it
has to make that option available.
--
MST
^ permalink raw reply
* Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging
From: Dmitry Torokhov @ 2011-11-05 6:47 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering, joe,
jkosina
In-Reply-To: <1319841316-29136-1-git-send-email-kys@microsoft.com>
Hi KY,
On Fri, Oct 28, 2011 at 03:35:16PM -0700, K. Y. Srinivasan wrote:
> The hv_mouse.c implements a hid compliant mouse driver for use on a
> Hyper-V based system. This driver is currently in the staging area and
> as part of the effort to move this driver out of staging, I had posted
> the driver code for community review a few weeks ago. This current patch
> addresses all the review comments I have gotten to date. All the relevant
> patches have already been submitted to the staging tree as well.
>
> As per Greg's suggestion, this patch does not get rid of the code from
> the staging area. Once the mouse driver lands under the hid directory,
> we will cleanup the staging directory.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
> drivers/hid/Kconfig | 6 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-hyperv.c | 600 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 607 insertions(+), 0 deletions(-)
> create mode 100644 drivers/hid/hid-hyperv.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 1130a89..068dd97 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -613,6 +613,12 @@ config HID_ZYDACRON
> ---help---
> Support for Zydacron remote control.
>
> +config HYPERV_MOUSE
> + tristate "Microsoft Hyper-V mouse driver"
> + depends on HYPERV
> + ---help---
> + Select this option to enable the Hyper-V mouse driver.
> +
> endmenu
>
> endif # HID_SUPPORT
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 0a0a38e..e683b8c 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_HID_ZYDACRON) += hid-zydacron.o
> obj-$(CONFIG_HID_WACOM) += hid-wacom.o
> obj-$(CONFIG_HID_WALTOP) += hid-waltop.o
> obj-$(CONFIG_HID_WIIMOTE) += hid-wiimote.o
> +obj-$(CONFIG_HYPERV_MOUSE) += hid-hyperv.o
>
> obj-$(CONFIG_USB_HID) += usbhid/
> obj-$(CONFIG_USB_MOUSE) += usbhid/
> diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
> new file mode 100644
> index 0000000..2c2e1b4
> --- /dev/null
> +++ b/drivers/hid/hid-hyperv.c
> @@ -0,0 +1,600 @@
> +/*
> + * Copyright (c) 2009, Citrix Systems, Inc.
> + * Copyright (c) 2010, Microsoft Corporation.
> + * Copyright (c) 2011, Novell Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/completion.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/hiddev.h>
> +#include <linux/hyperv.h>
> +
> +
> +struct hv_input_dev_info {
> + unsigned int size;
> + unsigned short vendor;
> + unsigned short product;
> + unsigned short version;
> + unsigned short reserved[11];
> +};
> +
> +/* The maximum size of a synthetic input message. */
> +#define SYNTHHID_MAX_INPUT_REPORT_SIZE 16
> +
> +/*
> + * Current version
> + *
> + * History:
> + * Beta, RC < 2008/1/22 1,0
> + * RC > 2008/1/22 2,0
> + */
> +#define SYNTHHID_INPUT_VERSION_MAJOR 2
> +#define SYNTHHID_INPUT_VERSION_MINOR 0
> +#define SYNTHHID_INPUT_VERSION (SYNTHHID_INPUT_VERSION_MINOR | \
> + (SYNTHHID_INPUT_VERSION_MAJOR << 16))
> +
> +
> +#pragma pack(push, 1)
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synthhid_msg_type {
> + SYNTH_HID_PROTOCOL_REQUEST,
> + SYNTH_HID_PROTOCOL_RESPONSE,
> + SYNTH_HID_INITIAL_DEVICE_INFO,
> + SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
> + SYNTH_HID_INPUT_REPORT,
> + SYNTH_HID_MAX
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synthhid_msg_hdr {
> + enum synthhid_msg_type type;
> + u32 size;
> +};
> +
> +struct synthhid_msg {
> + struct synthhid_msg_hdr header;
> + char data[1]; /* Enclosed message */
> +};
> +
> +union synthhid_version {
> + struct {
> + u16 minor_version;
> + u16 major_version;
> + };
> + u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synthhid_protocol_request {
> + struct synthhid_msg_hdr header;
> + union synthhid_version version_requested;
> +};
> +
> +struct synthhid_protocol_response {
> + struct synthhid_msg_hdr header;
> + union synthhid_version version_requested;
> + unsigned char approved;
> +};
> +
> +struct synthhid_device_info {
> + struct synthhid_msg_hdr header;
> + struct hv_input_dev_info hid_dev_info;
> + struct hid_descriptor hid_descriptor;
> +};
> +
> +struct synthhid_device_info_ack {
> + struct synthhid_msg_hdr header;
> + unsigned char reserved;
> +};
> +
> +struct synthhid_input_report {
> + struct synthhid_msg_hdr header;
> + char buffer[1];
> +};
> +
> +#pragma pack(pop)
> +
> +#define INPUTVSC_SEND_RING_BUFFER_SIZE (10*PAGE_SIZE)
> +#define INPUTVSC_RECV_RING_BUFFER_SIZE (10*PAGE_SIZE)
> +
> +
> +enum pipe_prot_msg_type {
> + PIPE_MESSAGE_INVALID,
> + PIPE_MESSAGE_DATA,
> + PIPE_MESSAGE_MAXIMUM
> +};
> +
> +
> +struct pipe_prt_msg {
> + enum pipe_prot_msg_type type;
> + u32 size;
> + char data[1];
> +};
> +
> +struct mousevsc_prt_msg {
> + enum pipe_prot_msg_type type;
> + u32 size;
> + union {
> + struct synthhid_protocol_request request;
> + struct synthhid_protocol_response response;
> + struct synthhid_device_info_ack ack;
> + };
> +};
> +
> +/*
> + * Represents an mousevsc device
> + */
> +struct mousevsc_dev {
> + struct hv_device *device;
> + bool init_complete;
> + bool connected;
> + struct mousevsc_prt_msg protocol_req;
> + struct mousevsc_prt_msg protocol_resp;
> + /* Synchronize the request/response if needed */
> + struct completion wait_event;
> + int dev_info_status;
> +
> + struct hid_descriptor *hid_desc;
> + unsigned char *report_desc;
> + u32 report_desc_size;
> + struct hv_input_dev_info hid_dev_info;
> + struct hid_device *hid_device;
> +};
> +
> +
> +static struct mousevsc_dev *alloc_input_device(struct hv_device *device)
Can we call it mousevsc_alloc_device?
> +{
> + struct mousevsc_dev *input_dev;
> +
> + input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL);
> +
> + if (!input_dev)
> + return NULL;
> +
> + input_dev->device = device;
> + hv_set_drvdata(device, input_dev);
> + init_completion(&input_dev->wait_event);
> +
> + return input_dev;
> +}
> +
> +static void free_input_device(struct mousevsc_dev *device)
Can we call it mousevsc_free_device?
> +{
> + kfree(device->hid_desc);
> + kfree(device->report_desc);
> + hv_set_drvdata(device->device, NULL);
> + kfree(device);
> +}
> +
> +
> +static void mousevsc_on_receive_device_info(struct mousevsc_dev *input_device,
> + struct synthhid_device_info *device_info)
> +{
> + int ret = 0;
> + struct hid_descriptor *desc;
> + struct mousevsc_prt_msg ack;
This could probably be:
static const mousevsc_prt_msg ack = {
.type = PIPE_MESSAGE_DATA,
.size = sizeof(struct synthhid_device_info_ack),
.ack = {
.header = {
.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK,
.size = 1,
},
.reserved = 0,
},
};
> +
> + /* Assume success for now */
> + input_device->dev_info_status = 0;
> +
> + memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info,
> + sizeof(struct hv_input_dev_info));
> +
or simply:
input_device->hid_dev_info = device_info->hid_dev_info;
> + desc = &device_info->hid_descriptor;
> + WARN_ON(desc->bLength == 0);
Should also goto cleanup as such descriptor is not usable.
> +
> + input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC);
> +
> + if (!input_device->hid_desc)
if you do
input_device->dev_info_status = -ENOMEM;
you could use it later instead of defaulting to -ENOMEM.
> + goto cleanup;
> +
> + memcpy(input_device->hid_desc, desc, desc->bLength);
> +
> + input_device->report_desc_size = desc->desc[0].wDescriptorLength;
> + if (input_device->report_desc_size == 0)
input_device->dev_info_status = -EINVAL;
> + goto cleanup;
> + input_device->report_desc = kzalloc(input_device->report_desc_size,
> + GFP_ATOMIC);
> +
> + if (!input_device->report_desc)
input_device->dev_info_status = -ENOMEM;
> + goto cleanup;
> +
> + memcpy(input_device->report_desc,
> + ((unsigned char *)desc) + desc->bLength,
> + desc->desc[0].wDescriptorLength);
> +
> + /* Send the ack */
> + memset(&ack, 0, sizeof(struct mousevsc_prt_msg));
> +
> + ack.type = PIPE_MESSAGE_DATA;
> + ack.size = sizeof(struct synthhid_device_info_ack);
> +
> + ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK;
> + ack.ack.header.size = 1;
> + ack.ack.reserved = 0;
> +
> + ret = vmbus_sendpacket(input_device->device->channel,
> + &ack,
> + sizeof(struct pipe_prt_msg) - sizeof(unsigned char) +
> + sizeof(struct synthhid_device_info_ack),
> + (unsigned long)&ack,
> + VM_PKT_DATA_INBAND,
> + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (ret != 0)
input_device->dev_info_status = ret;
> + goto cleanup;
> +
> + complete(&input_device->wait_event);
> +
> + return;
> +
> +cleanup:
> + kfree(input_device->hid_desc);
> + input_device->hid_desc = NULL;
> +
> + kfree(input_device->report_desc);
> + input_device->report_desc = NULL;
Do you have to clean it up here? You still need to clean it up in main
unwind path so consolidate both error and success path and just signal
completion and let main code figure it all out.
> +
> + input_device->dev_info_status = -1;
> + complete(&input_device->wait_event);
> +}
> +
> +static void mousevsc_on_receive(struct hv_device *device,
> + struct vmpacket_descriptor *packet)
> +{
> + struct pipe_prt_msg *pipe_msg;
> + struct synthhid_msg *hid_msg;
> + struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> + struct synthhid_input_report *input_report;
> +
> + pipe_msg = (struct pipe_prt_msg *)((unsigned long)packet +
> + (packet->offset8 << 3));
> +
> + if (pipe_msg->type != PIPE_MESSAGE_DATA)
> + return;
> +
> + hid_msg = (struct synthhid_msg *)&pipe_msg->data[0];
> +
> + switch (hid_msg->header.type) {
> + case SYNTH_HID_PROTOCOL_RESPONSE:
> + /*
> + * While it will be impossible for us to protect against
> + * malicious/buggy hypervisor/host, add a check here to
> + * ensure we don't corrupt memory.
> + */
> + if ((pipe_msg->size + sizeof(struct pipe_prt_msg)
> + - sizeof(unsigned char))
> + > sizeof(struct mousevsc_prt_msg)) {
> + WARN_ON(1);
> + break;
> + }
> +
> + memcpy(&input_dev->protocol_resp, pipe_msg,
> + pipe_msg->size + sizeof(struct pipe_prt_msg) -
> + sizeof(unsigned char));
> + complete(&input_dev->wait_event);
> + break;
> +
> + case SYNTH_HID_INITIAL_DEVICE_INFO:
> + WARN_ON(pipe_msg->size < sizeof(struct hv_input_dev_info));
> +
> + /*
> + * Parse out the device info into device attr,
> + * hid desc and report desc
> + */
> + mousevsc_on_receive_device_info(input_dev,
> + (struct synthhid_device_info *)&pipe_msg->data[0]);
&pipe_msg->data[0] is the same as pipe_msg->data, but the latter is
shorter and cleaner.
> + break;
> + case SYNTH_HID_INPUT_REPORT:
> + input_report =
> + (struct synthhid_input_report *)&pipe_msg->data[0];
> + if (!input_dev->init_complete)
> + break;
> + hid_input_report(input_dev->hid_device,
> + HID_INPUT_REPORT, input_report->buffer,
> + input_report->header.size, 1);
> + break;
> + default:
> + pr_err("unsupported hid msg type - type %d len %d",
> + hid_msg->header.type, hid_msg->header.size);
> + break;
> + }
> +
> +}
> +
> +static void mousevsc_on_channel_callback(void *context)
> +{
> + const int packet_size = 0x100;
> + int ret;
> + struct hv_device *device = context;
> + u32 bytes_recvd;
> + u64 req_id;
> + struct vmpacket_descriptor *desc;
> + unsigned char *buffer;
> + int bufferlen = packet_size;
> +
> + buffer = kmalloc(bufferlen, GFP_ATOMIC);
> + if (!buffer)
> + return;
> +
> + do {
> + ret = vmbus_recvpacket_raw(device->channel, buffer,
> + bufferlen, &bytes_recvd, &req_id);
> +
> + switch (ret) {
> + case 0:
> + if (bytes_recvd <= 0) {
> + kfree(buffer);
> + return;
> + }
> + desc = (struct vmpacket_descriptor *)buffer;
> +
> + switch (desc->type) {
> + case VM_PKT_COMP:
> + break;
> +
> + case VM_PKT_DATA_INBAND:
> + mousevsc_on_receive(device, desc);
> + break;
> +
> + default:
> + pr_err("unhandled packet type %d, tid %llx len %d\n",
> + desc->type,
> + req_id,
> + bytes_recvd);
> + break;
> + }
> +
> + break;
> +
> + case -ENOBUFS:
> + kfree(buffer);
> + /* Handle large packet */
> + bufferlen = bytes_recvd;
> + buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
Instead of potentially ever-increasing buffer that you also allocate
(and it looks like leaking on every callback invocation) can you just
repeat the read if you know that there are more data and use single
pre-allocated buffer?
> +
> + if (!buffer)
> + return;
> +
> + break;
> + }
> + } while (1);
> +
> +}
> +
> +static int mousevsc_connect_to_vsp(struct hv_device *device)
> +{
> + int ret = 0;
> + int t;
> + struct mousevsc_dev *input_dev = hv_get_drvdata(device);
> + struct mousevsc_prt_msg *request;
> + struct mousevsc_prt_msg *response;
> +
> + request = &input_dev->protocol_req;
> +
> + memset(request, 0, sizeof(struct mousevsc_prt_msg));
> +
> + request->type = PIPE_MESSAGE_DATA;
> + request->size = sizeof(struct synthhid_protocol_request);
> +
> + request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST;
> + request->request.header.size = sizeof(unsigned int);
> + request->request.version_requested.version = SYNTHHID_INPUT_VERSION;
>
This is constant data; just do the same as with ack in
on_receive_device_info. It does not need to be a part of input_dev, does
it?
> + ret = vmbus_sendpacket(device->channel, request,
> + sizeof(struct pipe_prt_msg) -
> + sizeof(unsigned char) +
> + sizeof(struct synthhid_protocol_request),
> + (unsigned long)request,
> + VM_PKT_DATA_INBAND,
> + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (ret)
> + goto cleanup;
> +
> + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
> + if (!t) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
> + }
> +
> + response = &input_dev->protocol_resp;
> +
> + if (!response->response.approved) {
> + pr_err("synthhid protocol request failed (version %d)\n",
> + SYNTHHID_INPUT_VERSION);
> + ret = -ENODEV;
> + goto cleanup;
> + }
> +
> + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ);
5 seconds? That's a long time of HV to respond...
> + if (!t) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
> + }
> +
> + /*
> + * We should have gotten the device attr, hid desc and report
> + * desc at this point
> + */
> + if (input_dev->dev_info_status)
> + ret = -ENOMEM;
Just do
ret = input_dev->dev_info_status;
unconditionally.
> +
> +cleanup:
> +
> + return ret;
> +}
> +
> +static int mousevsc_hid_open(struct hid_device *hid)
> +{
> + return 0;
> +}
> +
> +static int mousevsc_hid_start(struct hid_device *hid)
> +{
> + return 0;
> +}
> +
> +static void mousevsc_hid_close(struct hid_device *hid)
> +{
> +}
> +
> +static void mousevsc_hid_stop(struct hid_device *hid)
> +{
> +}
> +
> +static struct hid_ll_driver mousevsc_ll_driver = {
> + .open = mousevsc_hid_open,
> + .close = mousevsc_hid_close,
> + .start = mousevsc_hid_start,
> + .stop = mousevsc_hid_stop,
> +};
> +
> +static struct hid_driver mousevsc_hid_driver;
> +
> +static int mousevsc_probe(struct hv_device *device,
> + const struct hv_vmbus_device_id *dev_id)
> +{
> + int ret = 0;
> + struct mousevsc_dev *input_dev;
> + struct hid_device *hid_dev;
> +
> + input_dev = alloc_input_device(device);
> +
> + if (!input_dev)
> + return -ENOMEM;
> +
> + input_dev->init_complete = false;
Should this also be in alloc_input_device()?
> +
> + ret = vmbus_open(device->channel,
> + INPUTVSC_SEND_RING_BUFFER_SIZE,
> + INPUTVSC_RECV_RING_BUFFER_SIZE,
> + NULL,
> + 0,
> + mousevsc_on_channel_callback,
> + device
> + );
> +
This blank line is extra IMO.
> + if (ret != 0) {
> + free_input_device(input_dev);
> + return ret;
Please do not mix error unwinding with gotos and unwinding in error
handling branches.
> + }
> +
> + ret = mousevsc_connect_to_vsp(device);
> +
This blank line is extra IMO.
> + if (ret != 0)
> + goto probe_err0;
> +
> + /* workaround SA-167 */
> + if (input_dev->report_desc[14] == 0x25)
> + input_dev->report_desc[14] = 0x29;
> +
> + hid_dev = hid_allocate_device();
> + if (IS_ERR(hid_dev)) {
> + ret = PTR_ERR(hid_dev);
> + goto probe_err0;
> + }
> +
> + hid_dev->ll_driver = &mousevsc_ll_driver;
> + hid_dev->driver = &mousevsc_hid_driver;
You are not really hid driver; you are more of a "provider" so why do
you need to set hid_dev->driver in addition to hid_dev->ll_driver?
> + hid_dev->bus = BUS_VIRTUAL;
> + hid_dev->vendor = input_dev->hid_dev_info.vendor;
> + hid_dev->product = input_dev->hid_dev_info.product;
> + hid_dev->version = input_dev->hid_dev_info.version;
> + input_dev->hid_device = hid_dev;
> +
> + sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
strlcpy?
> +
> + ret = hid_parse_report(hid_dev, input_dev->report_desc,
> + input_dev->report_desc_size);
> +
> + if (ret) {
> + hid_err(hid_dev, "parse failed\n");
> + goto probe_err1;
> + }
> +
> + ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
Why do you need to call hid_hw_start instead of letting HID core figure
it out for you?
> +
> + if (ret) {
> + hid_err(hid_dev, "hw start failed\n");
> + goto probe_err1;
> + }
> +
> + input_dev->connected = true;
> + input_dev->init_complete = true;
> +
> + return ret;
> +
> +probe_err1:
> + hid_destroy_device(hid_dev);
> +
> +probe_err0:
> + vmbus_close(device->channel);
> + free_input_device(input_dev);
> + return ret;
> +}
> +
> +
> +static int mousevsc_remove(struct hv_device *dev)
> +{
> + struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
> +
> + vmbus_close(dev->channel);
> +
> + if (input_dev->connected) {
Can we even get here if input_dev->connected == false?
> + hidinput_disconnect(input_dev->hid_device);
You did not explicitly connect hidinput; leave disconnecting to the HID
core as well.
It looks like all that is needed is:
static int mousevsc_remove(struct hv_device *dev)
{
struct mousevsc_dev *mouse = hv_get_drvdata(dev);
vmbus_close(dev->channel);
hid_destroy_device(mouse->hid_device);
mousevcs_free_device(mouse);
return 0;
}
> + input_dev->connected = false;
> + hid_destroy_device(input_dev->hid_device);
> + }
> +
> + free_input_device(input_dev);
> +
> + return 0;
> +}
> +
> +static const struct hv_vmbus_device_id id_table[] = {
> + /* Mouse guid */
> + { VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c,
> + 0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(vmbus, id_table);
> +
> +static struct hv_driver mousevsc_drv = {
> + .name = KBUILD_MODNAME,
> + .id_table = id_table,
> + .probe = mousevsc_probe,
> + .remove = mousevsc_remove,
> +};
> +
> +static int __init mousevsc_init(void)
> +{
> + return vmbus_driver_register(&mousevsc_drv);
> +}
> +
> +static void __exit mousevsc_exit(void)
> +{
> + vmbus_driver_unregister(&mousevsc_drv);
> +}
> +
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(HV_DRV_VERSION);
> +module_init(mousevsc_init);
> +module_exit(mousevsc_exit);
> --
> 1.7.4.1
>
Thanks.
--
Dmitry
^ permalink raw reply
* CFP: 12th IEEE/ACM International Symposium on Cluster, Grid and Cloud Computing (CCGrid 2012) Ottawa, Canada
From: Ioan Raicu @ 2011-11-05 2:39 UTC (permalink / raw)
To: virtualization
CALL FOR PAPERS
12th IEEE/ACM International Symposium on Cluster, Grid and Cloud
Computing (CCGrid 2012) Ottawa, Canada
May 13-16, 2012
http://www.cloudbus.org/ccgrid2012
Rapid advances in processing, communication and systems/middleware
technologies are leading to new paradigms and platforms for computing,
ranging from computing Clusters to widely distributed Grid and
emerging Clouds. CCGrid is a series of very successful conferences,
sponsored by the IEEE Computer Society Technical Committee on Scalable
Computing (TCSC) and ACM, with the overarching goal of bringing
together international researchers, developers, and users and to
provide an international forum to present leading research activities
and results on a broad range of topics related to these platforms and
paradigms and their applications. The conference features keynotes,
technical presentations, posters and research demos, workshops,
tutorials, as well as the SCALE challenges featuring live
demonstrations.
In 2012, CCGrid will come to Canada for the first time and will be
held in Ottawa, the capital city. CCGrid 2012 will have a focus on
important and immediate issues that are significantly influencing all
aspects of cluster, cloud and grid computing. Topics of interest
include, but are not limited to:
* Applications and Experiences: Applications to real and complex
problems in science, engineering, business and society; User
studies; Experiences with large-scale deployments systems or
applications.
* Architecture: System architectures, Design and deployment.
* Autonomic Computing and Cyberinfrastructure: Self managed behavior,
models and technologies; Autonomic paradigms and approaches
(control-based, bio-inspired, emergent, etc.); Bio-inspired
approaches to management; SLA definition and enforcement.
* Performance Modeling and Evaluation: Performance models; Monitoring
and evaluation tools, Analysis of system/application performance;
Benchmarks and testbeds.
* Programming Models, Systems, and Fault-Tolerant Computing:
Programming models for cluster, clouds and grid computing; fault
tolerant infrastructure and algorithms; systems software to enable
efficient computing.
* Multicore and Accelerator-based Computing: Software and application
techniques to utilize multicore architectures and
accelerators/heterogeneous computing systems.
* Scheduling and Resource Management: Techniques to schedule jobs and
resources on clusters, clouds and grid computing platforms.
* Cloud Computing: Cloud architectures; Software tools and techniques
for clouds.
PAPER SUBMISSION
Authors are invited to submit papers electronically. Submitted
manuscripts should be structured as technical papers and may not
exceed 8 letter size (8.5 x 11) pages including figures, tables and
references using the IEEE format for conference proceedings (print
area of 6-1/2 inches (16.51 cm) wide by 8-7/8 inches (22.51 cm) high,
two-column format with columns 3-1/16 inches (7.85 cm) wide with a 3/8
inch (0.81 cm) space between them, single-spaced 10-point Times fully
justified text). Submissions not conforming to these guidelines may be
returned without review. Authors should submit the manuscript in PDF
format and make sure that the file will print on a printer that uses
letter size (8.5 x 11) paper. The official language of the meeting is
English. All manuscripts will be reviewed and will be judged on
correctness, originality, technical strength, significance, quality of
presentation, and interest and relevance to the conference attendees.
Submitted papers must represent original unpublished research that is
not currently under review for any other conference or journal. Papers
not following these guidelines will be rejected without review and
further action may be taken, including (but not limited to)
notifications sent to the heads of the institutions of the authors and
sponsors of the conference. Submissions received after the due date,
exceeding the page limit, or not appropriately structured may not be
considered. Authors may contact the conference chairs for more
information. The proceedings will be published through the IEEE
Computer Society Press, USA and will be made available online through
the IEEE Digital Library.
Submission Link:
https://www.easychair.org/account/signin.cgi?conf=ccgrid2012
JOURNAL SPECIAL ISSUE
Highly rated Top 6 papers from the CCGrid 2012 conference will be
invited to extend for publication in a special issue of the "Future
Generation Computer Systems (FGCS)" Journal published by
Elsevier Press.
CHAIRS
General Chair
* Shikharesh Majumdar, Carleton University, Canada
Honorary Chair
* Geoffrey Fox, Indiana University, USA
Program Committee Co-Chairs
* Rajkumar Buyya, University of Melbourne, Australia
* Pavan Balaji, Argonne National Laboratory, USA
Program Committee Vice-chairs
* Daniel S. Katz (Applications and Experiences)
* Dhabaleswar K. Panda (Architecture)
* Manish Parashar (Middleware, Autonomic Computing, and Cyberinfrastructure)
* Ahmad Afsahi (Performance Modeling and Analysis)
* Xian-He Sun (Performance Measurement and Evaluation)
* William Gropp (Programming Models, Systems, and Fault-Tolerant computing)
* David Bader (Multicore and Accelerator-based Computing)
* Thomas Fahringer (Scheduling and Resource Management)
* Ignacio Martin Llorente and Madhusudhan Govindaraju (Cloud Computing)
Cyber Co-Chairs
* Anton Beloglazov, The University of Melbourne, Australia
* Suraj Pandey, CSIRO, Australia
* Trevor Gelowsky, Carleton University, Canada
Workshops Co-Chairs
* Marin Litiou, York University, Canada
* Mukaddim Pathan, Telstra Corporation Limited, Australia
Publicity Chairs
* Helen Karatza, Aristotle University of Thessaloniki, Greece
* Ioan Raicu, Illinois Institute of Technology& Argonne National Labs, USA
* Bruno Schulze, National Laboratory for Scientific Computing, Brazil
* G Subrahmanya VRK Rao: Cognizant technology Solutions, India
Tutorials Co-Chairs
* Sushil K. Prasad, Georgia State University, USA
* Rob Simmonds, Westgrid, Canada
Doctoral Symposium Co-Chairs
* Carlos Varela, Rensselaer Polytechnic Institute, USA
* Yogesh Simmhan, University of Southern California
Poster and Research Demo Co-Chairs
* Suraj Pandey, CSIRO, Australia
SCALE Challenge Coordinator
* Shantenu Jha, Rutgers and Loisiana State University
Steering Committee
* Henri Bal, Vrije University, The Netherlands
* Pavan Balaji, Argonne National Laboratory, USA
* Rajkumar Buyya, University of Melbourne, Australia (Chair)
* Franck Capello, University of Paris-Sud, France
* Jack Dongarra, University of Tennessee& ORNL, USA
* Dick Epema, Technical University of Delft, The Netherlands
* Thomas Fahringer, University of Innsbruck, Austria
* Ian Foster, University of Chicago, USA
* Wolfgang Gentzsch, DEISA, Germany
* Hai Jin, Huazhong University of Science& Technology, China
* Craig Lee, The Aerospace Corporation, USA (Co-Chair)
* Laurent Lefevre, INRIA, France
* Geng Lin, Dell Inc., USA
* Manish Parashar, Rutgers: The State University of New Jersey, USA
* Shikharesh Majumdar, Carleton University, Canada
* Satoshi Matsuoaka, Tokyo Institute of Technology, Japan
* Omer Rana, Cardiff University, UK
* Paul Roe, Queensland University of Technology, Australia
* Bruno Schulze, LNCC, Brazil
* Nalini Venkatasubramanian, University of California, USA
* Carlos Varela, Rensselaer Polytechnic Institute, USA
IMPORTANT DATES
Papers Due: 25 November 2011
Notification of Acceptance: 30 January 2012
Camera Ready Papers Due: 27 February 2012
Sponsors: IEEE Computer Society (TCSE)& ACM SIGARCH (approval pending)
--
=================================================================
Ioan Raicu, Ph.D.
Assistant Professor, Illinois Institute of Technology (IIT)
Guest Research Faculty, Argonne National Laboratory (ANL)
=================================================================
Data-Intensive Distributed Systems Laboratory, CS/IIT
Distributed Systems Laboratory, MCS/ANL
=================================================================
Cel: 1-847-722-0876
Office: 1-312-567-5704
Email: iraicu@cs.iit.edu
Web: http://www.cs.iit.edu/~iraicu/
Web: http://datasys.cs.iit.edu/
=================================================================
=================================================================
^ permalink raw reply
* Re: [patch 1/2] xen-gntalloc: integer overflow in gntalloc_ioctl_alloc()
From: Konrad Rzeszutek Wilk @ 2011-11-04 18:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, kernel-janitors, xen-devel, virtualization
In-Reply-To: <20111104182408.GD5796@elgon.mountain>
On Fri, Nov 04, 2011 at 09:24:08PM +0300, Dan Carpenter wrote:
> On 32 bit systems a high value of op.count could lead to an integer
> overflow in the kzalloc() and gref_ids would be smaller than
> expected. If the you triggered another integer overflow in
> "if (gref_size + op.count > limit)" then you'd probably get memory
> corruption inside add_grefs().
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Great! Keep them coming! Will push for stable and 3.2.
>
> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
> index f6832f4..23c60cf 100644
> --- a/drivers/xen/gntalloc.c
> +++ b/drivers/xen/gntalloc.c
> @@ -280,7 +280,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
> goto out;
> }
>
> - gref_ids = kzalloc(sizeof(gref_ids[0]) * op.count, GFP_TEMPORARY);
> + gref_ids = kcalloc(op.count, sizeof(gref_ids[0]), GFP_TEMPORARY);
> if (!gref_ids) {
> rc = -ENOMEM;
> goto out;
^ permalink raw reply
* [patch 2/2] xen-gntalloc: signedness bug in add_grefs()
From: Dan Carpenter @ 2011-11-04 18:24 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: kernel-janitors, xen-devel, virtualization, Konrad Rzeszutek Wilk
gref->gref_id is unsigned so the error handling didn't work.
gnttab_grant_foreign_access() returns an int type, so we can add a
cast here, and it doesn't cause any problems.
gnttab_grant_foreign_access() can return a variety of errors
including -ENOSPC, -ENOSYS and -ENOMEM.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index 23c60cf..e1c4c6e 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -135,7 +135,7 @@ static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
/* Grant foreign access to the page. */
gref->gref_id = gnttab_grant_foreign_access(op->domid,
pfn_to_mfn(page_to_pfn(gref->page)), readonly);
- if (gref->gref_id < 0) {
+ if ((int)gref->gref_id < 0) {
rc = gref->gref_id;
goto undo;
}
^ permalink raw reply related
* [patch 1/2] xen-gntalloc: integer overflow in gntalloc_ioctl_alloc()
From: Dan Carpenter @ 2011-11-04 18:24 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: kernel-janitors, xen-devel, virtualization, Konrad Rzeszutek Wilk
On 32 bit systems a high value of op.count could lead to an integer
overflow in the kzalloc() and gref_ids would be smaller than
expected. If the you triggered another integer overflow in
"if (gref_size + op.count > limit)" then you'd probably get memory
corruption inside add_grefs().
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
index f6832f4..23c60cf 100644
--- a/drivers/xen/gntalloc.c
+++ b/drivers/xen/gntalloc.c
@@ -280,7 +280,7 @@ static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
goto out;
}
- gref_ids = kzalloc(sizeof(gref_ids[0]) * op.count, GFP_TEMPORARY);
+ gref_ids = kcalloc(op.count, sizeof(gref_ids[0]), GFP_TEMPORARY);
if (!gref_ids) {
rc = -ENOMEM;
goto out;
^ permalink raw reply related
* [patch] xen-gntdev: integer overflow in gntdev_alloc_map()
From: Dan Carpenter @ 2011-11-04 18:23 UTC (permalink / raw)
To: Jeremy Fitzhardinge
Cc: kernel-janitors, xen-devel, virtualization, Konrad Rzeszutek Wilk
The multiplications here can overflow resulting in smaller buffer
sizes than expected. "count" comes from a copy_from_user().
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 880798a..2f3c363 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -114,11 +114,11 @@ static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
if (NULL == add)
return NULL;
- add->grants = kzalloc(sizeof(add->grants[0]) * count, GFP_KERNEL);
- add->map_ops = kzalloc(sizeof(add->map_ops[0]) * count, GFP_KERNEL);
- add->unmap_ops = kzalloc(sizeof(add->unmap_ops[0]) * count, GFP_KERNEL);
- add->kmap_ops = kzalloc(sizeof(add->kmap_ops[0]) * count, GFP_KERNEL);
- add->pages = kzalloc(sizeof(add->pages[0]) * count, GFP_KERNEL);
+ add->grants = kcalloc(count, sizeof(add->grants[0]), GFP_KERNEL);
+ add->map_ops = kcalloc(count, sizeof(add->map_ops[0]), GFP_KERNEL);
+ add->unmap_ops = kcalloc(count, sizeof(add->unmap_ops[0]), GFP_KERNEL);
+ add->kmap_ops = kcalloc(count, sizeof(add->kmap_ops[0]), GFP_KERNEL);
+ add->pages = kcalloc(count, sizeof(add->pages[0]), GFP_KERNEL);
if (NULL == add->grants ||
NULL == add->map_ops ||
NULL == add->unmap_ops ||
^ permalink raw reply related
* Re: virtio-pci new configuration proposal
From: Sasha Levin @ 2011-11-04 14:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <20111104142338.GB24452@redhat.com>
On Fri, 2011-11-04 at 16:23 +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 04, 2011 at 03:53:24PM +0200, Sasha Levin wrote:
> > On Fri, 2011-11-04 at 15:51 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Nov 04, 2011 at 02:32:19PM +0200, Sasha Levin wrote:
> > > > On Fri, 2011-11-04 at 13:40 +0200, Michael S. Tsirkin wrote:
> > > > > On Fri, Nov 04, 2011 at 08:14:43PM +1030, Rusty Russell wrote:
> > > > > > > > 3) If we're changing the queue layout, it's a chance to fix a
> > > > > > > > longstanding bug: let the guest notify the host of preferred
> > > > > > > > queue size and alignment.
> > > > > > >
> > > > > > > Yup, we can do that.
> > > > >
> > > > > We don't need to change all of layout for that - just add another field
> > > > > in the common config structure to supply the alignment.
> > > >
> > > > How would you do it without changing the layout? Add another optional
> > > > field at the end which will shift offsets based on whether the host and
> > > > guest support this new feature or not?
> > > >
> > > > This leads to 3 different things which now shift config offsets around.
> > >
> > > No. Just put the field at offset 24 from the offset specified
> > > by VIRTIO_PCI_CAP_COMMON_CFG.
> >
> > Two questions here:
> >
> > - What about backwards compatibility? How would the config space look
> > when we're not using the new layout?
>
> Exactly as it does now. You don't get to tweak alignment then.
>
> > - How does it work with 64 bit features which are also located there?
>
> Basically each field gets an offset. E.g.
> 24 - features
> 28 - queue alignment
>
> > > > As you said, the PCI cap list was introduced both to save space (which
> > > > is not the motivation here), and because it's a very efficient
> > >
> > > It's actually pretty inefficient - there's an overhead of 3 bytes for
> > > each vendor specific option.
> >
> > It's efficient because while you pay a small price for each optional
> > option it also means that that option is optional and won't clutter the
> > config space if it's not really in use.
>
> I guess my assumption is that most options will be in use,
> not discarded dead-ends.
I don't know about that. 64 bit features would be pretty rare for now -
and I don't think that setting the alignment will be also enabled by
default.
I think that we're looking at it differently because I assume that any
feature we add at this point would be optional and used only in specific
scenarios, while you think that everything added will be used most of
the time.
> > Think of how the PCI config space would look if all those caps wouldn't
> > have been optional and would instead all of them would have just have
> > been attached to the end of the config space.
>
> It started out this way, but then they started running out
> of space - it's only 256 bytes - so the capability mechanism
> was invented.
>
>
> > >
> > > > and easy way to manage optional features without requiring tricks
> > > > which move offsets around like we do now.
> > >
> > > Tricks with offsets only appeared because we had datapath, device
> > > specific and common config in the same place.
> > > feature list isn't needed to fix that.
> > >
> > > > --
> > > >
> > > > Sasha.
> >
> > --
> >
> > Sasha.
--
Sasha.
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Michael S. Tsirkin @ 2011-11-04 14:23 UTC (permalink / raw)
To: Sasha Levin; +Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <1320414804.3334.13.camel@lappy>
On Fri, Nov 04, 2011 at 03:53:24PM +0200, Sasha Levin wrote:
> On Fri, 2011-11-04 at 15:51 +0200, Michael S. Tsirkin wrote:
> > On Fri, Nov 04, 2011 at 02:32:19PM +0200, Sasha Levin wrote:
> > > On Fri, 2011-11-04 at 13:40 +0200, Michael S. Tsirkin wrote:
> > > > On Fri, Nov 04, 2011 at 08:14:43PM +1030, Rusty Russell wrote:
> > > > > > > 3) If we're changing the queue layout, it's a chance to fix a
> > > > > > > longstanding bug: let the guest notify the host of preferred
> > > > > > > queue size and alignment.
> > > > > >
> > > > > > Yup, we can do that.
> > > >
> > > > We don't need to change all of layout for that - just add another field
> > > > in the common config structure to supply the alignment.
> > >
> > > How would you do it without changing the layout? Add another optional
> > > field at the end which will shift offsets based on whether the host and
> > > guest support this new feature or not?
> > >
> > > This leads to 3 different things which now shift config offsets around.
> >
> > No. Just put the field at offset 24 from the offset specified
> > by VIRTIO_PCI_CAP_COMMON_CFG.
>
> Two questions here:
>
> - What about backwards compatibility? How would the config space look
> when we're not using the new layout?
Exactly as it does now. You don't get to tweak alignment then.
> - How does it work with 64 bit features which are also located there?
Basically each field gets an offset. E.g.
24 - features
28 - queue alignment
> > > As you said, the PCI cap list was introduced both to save space (which
> > > is not the motivation here), and because it's a very efficient
> >
> > It's actually pretty inefficient - there's an overhead of 3 bytes for
> > each vendor specific option.
>
> It's efficient because while you pay a small price for each optional
> option it also means that that option is optional and won't clutter the
> config space if it's not really in use.
I guess my assumption is that most options will be in use,
not discarded dead-ends.
> Think of how the PCI config space would look if all those caps wouldn't
> have been optional and would instead all of them would have just have
> been attached to the end of the config space.
It started out this way, but then they started running out
of space - it's only 256 bytes - so the capability mechanism
was invented.
> >
> > > and easy way to manage optional features without requiring tricks
> > > which move offsets around like we do now.
> >
> > Tricks with offsets only appeared because we had datapath, device
> > specific and common config in the same place.
> > feature list isn't needed to fix that.
> >
> > > --
> > >
> > > Sasha.
>
> --
>
> Sasha.
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Sasha Levin @ 2011-11-04 13:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <20111104135113.GA24452@redhat.com>
On Fri, 2011-11-04 at 15:51 +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 04, 2011 at 02:32:19PM +0200, Sasha Levin wrote:
> > On Fri, 2011-11-04 at 13:40 +0200, Michael S. Tsirkin wrote:
> > > On Fri, Nov 04, 2011 at 08:14:43PM +1030, Rusty Russell wrote:
> > > > > > 3) If we're changing the queue layout, it's a chance to fix a
> > > > > > longstanding bug: let the guest notify the host of preferred
> > > > > > queue size and alignment.
> > > > >
> > > > > Yup, we can do that.
> > >
> > > We don't need to change all of layout for that - just add another field
> > > in the common config structure to supply the alignment.
> >
> > How would you do it without changing the layout? Add another optional
> > field at the end which will shift offsets based on whether the host and
> > guest support this new feature or not?
> >
> > This leads to 3 different things which now shift config offsets around.
>
> No. Just put the field at offset 24 from the offset specified
> by VIRTIO_PCI_CAP_COMMON_CFG.
Two questions here:
- What about backwards compatibility? How would the config space look
when we're not using the new layout?
- How does it work with 64 bit features which are also located there?
> > As you said, the PCI cap list was introduced both to save space (which
> > is not the motivation here), and because it's a very efficient
>
> It's actually pretty inefficient - there's an overhead of 3 bytes for
> each vendor specific option.
It's efficient because while you pay a small price for each optional
option it also means that that option is optional and won't clutter the
config space if it's not really in use.
Think of how the PCI config space would look if all those caps wouldn't
have been optional and would instead all of them would have just have
been attached to the end of the config space.
>
> > and easy way to manage optional features without requiring tricks
> > which move offsets around like we do now.
>
> Tricks with offsets only appeared because we had datapath, device
> specific and common config in the same place.
> feature list isn't needed to fix that.
>
> > --
> >
> > Sasha.
--
Sasha.
^ permalink raw reply
* Re: virtio-pci new configuration proposal
From: Michael S. Tsirkin @ 2011-11-04 13:51 UTC (permalink / raw)
To: Sasha Levin; +Cc: Anthony Liguori, linux-kernel, kvm, virtualization
In-Reply-To: <1320409939.3334.6.camel@lappy>
On Fri, Nov 04, 2011 at 02:32:19PM +0200, Sasha Levin wrote:
> On Fri, 2011-11-04 at 13:40 +0200, Michael S. Tsirkin wrote:
> > On Fri, Nov 04, 2011 at 08:14:43PM +1030, Rusty Russell wrote:
> > > > > 3) If we're changing the queue layout, it's a chance to fix a
> > > > > longstanding bug: let the guest notify the host of preferred
> > > > > queue size and alignment.
> > > >
> > > > Yup, we can do that.
> >
> > We don't need to change all of layout for that - just add another field
> > in the common config structure to supply the alignment.
>
> How would you do it without changing the layout? Add another optional
> field at the end which will shift offsets based on whether the host and
> guest support this new feature or not?
>
> This leads to 3 different things which now shift config offsets around.
No. Just put the field at offset 24 from the offset specified
by VIRTIO_PCI_CAP_COMMON_CFG.
> As you said, the PCI cap list was introduced both to save space (which
> is not the motivation here), and because it's a very efficient
It's actually pretty inefficient - there's an overhead of 3 bytes for
each vendor specific option.
> and easy way to manage optional features without requiring tricks
> which move offsets around like we do now.
Tricks with offsets only appeared because we had datapath, device
specific and common config in the same place.
feature list isn't needed to fix that.
> --
>
> Sasha.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox