* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-24 8:05 UTC (permalink / raw)
To: Avi Kivity; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <4BFA1E91.8060706@redhat.com>
On Mon, May 24, 2010 at 09:37:05AM +0300, Avi Kivity wrote:
> On 05/23/2010 07:30 PM, Michael S. Tsirkin wrote:
>>
>>
>>>> Maybe we should use atomics on index then?
>>>>
>>>>
>>> This should only be helpful if you access the cacheline several times in
>>> a row. That's not the case in virtio (or here).
>>>
>> So why does it help?
>>
>
> We actually do access the cacheline several times in a row here (but not
> in virtio?):
>
>> case SHARE:
>> while (count< MAX_BOUNCES) {
>> /* Spin waiting for other side to change it. */
>> while (counter->cacheline1 != count);
>>
>
> Broadcast a read request.
>
>> count++;
>> counter->cacheline1 = count;
>>
>
> Broadcast an invalidate request.
>
>> count++;
>> }
>> break;
>>
>> case LOCKSHARE:
>> while (count< MAX_BOUNCES) {
>> /* Spin waiting for other side to change it. */
>> while (__sync_val_compare_and_swap(&counter->cacheline1, count, count+1)
>> != count);
>>
>
> Broadcast a 'read for ownership' request.
>
>> count += 2;
>> }
>> break;
>>
>
> So RMW should certainly by faster using single-instruction RMW
> operations (or using prefetchw).
Okay, but why is lockunshare faster than unshare?
> --
> Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply
* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Avi Kivity @ 2010-05-24 11:00 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <20100524080518.GA16115@redhat.com>
On 05/24/2010 11:05 AM, Michael S. Tsirkin wrote:
>
> Okay, but why is lockunshare faster than unshare?
>
>
No idea.
--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.
^ permalink raw reply
* RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization
From: Haiyang Zhang @ 2010-05-25 23:14 UTC (permalink / raw)
To: Ky Srinivasan, Greg KH
Cc: 'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org',
'linux-kernel@vger.kernel.org'
In-Reply-To: <4BF7A20602000030000859E3@sinclair.provo.novell.com>
> From: Ky Srinivasan [mailto:ksrinivasan@novell.com]
> You would need to protect the increment, if interrupts are going to
> come in on any cpu and update the counter. While in your current
> implementation interrupts are only delivered on cpu0, it is still
> probably good to deal with the more general case and protect the
> counter.
>
> On a slightly different note, why don't you make the synchronization
> more explicit than what you currently have: Rather than polling the
> variable in a loop, why don't you put that context to sleep and the
> interrupt context that updates the count would be responsible for
> issuing the wakeup when the conditions are appropriate - when all
> channels are initialized.
Thank you for the suggestion. I will keep the counter atomic to handle
more general case potentially. To ensure channels are ready before
vmbus_init() returns, I used an event waiting mechanism instead of
polling the variable periodically. A modified patch will be submitted
soon.
Thanks,
- Haiyang
^ permalink raw reply
* [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Haiyang Zhang @ 2010-05-26 16:54 UTC (permalink / raw)
To: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
"'virtualization@lists.osdl.org'" <virtualiz>
[-- Attachment #1: Type: text/plain, Size: 3562 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: [PATCH] staging: hv: Fix race condition on IC channel initialization
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an event waiting to ensure all channels are ready before
vmbus_init() returns. So another module won't have any uninitialized channel.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel_mgmt.c | 23 +++++++++++++----------
drivers/staging/hv/vmbus_drv.c | 10 ++++++++++
drivers/staging/hv/vmbus_private.h | 1 +
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..f99db1b 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
int ret;
int cnt;
unsigned long flags;
+ static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
DPRINT_ENTER(VMBUS);
@@ -373,22 +374,24 @@ static void VmbusChannelProcessOffer(void *context)
* can cleanup properly
*/
newChannel->State = CHANNEL_OPEN_STATE;
- cnt = 0;
- while (cnt != MAX_MSG_TYPES) {
+ /* Open IC channels */
+ for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
&hv_cb_utils[cnt].data,
- sizeof(struct hv_guid)) == 0) {
+ sizeof(struct hv_guid)) == 0 &&
+ VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+ 2 * PAGE_SIZE, NULL, 0,
+ hv_cb_utils[cnt].callback,
+ newChannel) == 0) {
+ hv_cb_utils[cnt].channel = newChannel;
+ mb();
DPRINT_INFO(VMBUS, "%s",
hv_cb_utils[cnt].log_msg);
-
- if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
- 2 * PAGE_SIZE, NULL, 0,
- hv_cb_utils[cnt].callback,
- newChannel) == 0)
- hv_cb_utils[cnt].channel = newChannel;
+ if (atomic_inc_return(&ic_channel_initcnt) ==
+ MAX_MSG_TYPES)
+ osd_WaitEventSet(ic_channel_ready);
}
- cnt++;
}
}
DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..3ae8981 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -989,6 +989,8 @@ static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
};
MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
+struct osd_waitevent *ic_channel_ready;
+
static int __init vmbus_init(void)
{
int ret = 0;
@@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
if (!dmi_check_system(microsoft_hv_dmi_table))
return -ENODEV;
+ ic_channel_ready = osd_WaitEventCreate();
+ if (ic_channel_ready == NULL)
+ return -ENOMEM;
+
ret = vmbus_bus_init(VmbusInitialize);
+ /* Wait until all IC channels are initialized */
+ osd_WaitEventWait(ic_channel_ready);
+
+ kfree(ic_channel_ready);
DPRINT_EXIT(VMBUS_DRV);
return ret;
}
diff --git a/drivers/staging/hv/vmbus_private.h b/drivers/staging/hv/vmbus_private.h
index 588c667..3fb8dad 100644
--- a/drivers/staging/hv/vmbus_private.h
+++ b/drivers/staging/hv/vmbus_private.h
@@ -130,5 +130,6 @@ int VmbusSetEvent(u32 childRelId);
void VmbusOnEvents(void);
+extern struct osd_waitevent *ic_channel_ready;
#endif /* _VMBUS_PRIVATE_H_ */
--
1.6.3.2
[-- Attachment #2: 0525-Fix-race-condition-on-IC-channel-initialization.patch --]
[-- Type: application/octet-stream, Size: 3454 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: [PATCH] staging: hv: Fix race condition on IC channel initialization
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an event waiting to ensure all channels are ready before
vmbus_init() returns. So another module won't have any uninitialized channel.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel_mgmt.c | 23 +++++++++++++----------
drivers/staging/hv/vmbus_drv.c | 10 ++++++++++
drivers/staging/hv/vmbus_private.h | 1 +
3 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..f99db1b 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
int ret;
int cnt;
unsigned long flags;
+ static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
DPRINT_ENTER(VMBUS);
@@ -373,22 +374,24 @@ static void VmbusChannelProcessOffer(void *context)
* can cleanup properly
*/
newChannel->State = CHANNEL_OPEN_STATE;
- cnt = 0;
- while (cnt != MAX_MSG_TYPES) {
+ /* Open IC channels */
+ for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
&hv_cb_utils[cnt].data,
- sizeof(struct hv_guid)) == 0) {
+ sizeof(struct hv_guid)) == 0 &&
+ VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+ 2 * PAGE_SIZE, NULL, 0,
+ hv_cb_utils[cnt].callback,
+ newChannel) == 0) {
+ hv_cb_utils[cnt].channel = newChannel;
+ mb();
DPRINT_INFO(VMBUS, "%s",
hv_cb_utils[cnt].log_msg);
-
- if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
- 2 * PAGE_SIZE, NULL, 0,
- hv_cb_utils[cnt].callback,
- newChannel) == 0)
- hv_cb_utils[cnt].channel = newChannel;
+ if (atomic_inc_return(&ic_channel_initcnt) ==
+ MAX_MSG_TYPES)
+ osd_WaitEventSet(ic_channel_ready);
}
- cnt++;
}
}
DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index c21731a..3ae8981 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -989,6 +989,8 @@ static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
};
MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
+struct osd_waitevent *ic_channel_ready;
+
static int __init vmbus_init(void)
{
int ret = 0;
@@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
if (!dmi_check_system(microsoft_hv_dmi_table))
return -ENODEV;
+ ic_channel_ready = osd_WaitEventCreate();
+ if (ic_channel_ready == NULL)
+ return -ENOMEM;
+
ret = vmbus_bus_init(VmbusInitialize);
+ /* Wait until all IC channels are initialized */
+ osd_WaitEventWait(ic_channel_ready);
+
+ kfree(ic_channel_ready);
DPRINT_EXIT(VMBUS_DRV);
return ret;
}
diff --git a/drivers/staging/hv/vmbus_private.h b/drivers/staging/hv/vmbus_private.h
index 588c667..3fb8dad 100644
--- a/drivers/staging/hv/vmbus_private.h
+++ b/drivers/staging/hv/vmbus_private.h
@@ -130,5 +130,6 @@ int VmbusSetEvent(u32 childRelId);
void VmbusOnEvents(void);
+extern struct osd_waitevent *ic_channel_ready;
#endif /* _VMBUS_PRIVATE_H_ */
--
1.6.3.2
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCHv2-RFC 0/2] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-26 19:50 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
Here's a rewrite of the original patch with a new layout.
I haven't tested it yet so no idea how this performs, but
I think this addresses the cache bounce issue raised by Avi.
Posting for early flames/comments.
Generally, the Host end of the virtio ring doesn't need to see where
Guest is up to in consuming the ring. However, to completely understand
what's going on from the outside, this information must be exposed.
For example, host can reduce the number of interrupts by detecting
that the guest is currently handling previous buffers.
We add a feature bit so the guest can tell the host that it's writing
out the current value there, if it wants to use that.
This differs from original approach in that the used index
is put after avail index (they are typically written out together).
To avoid cache bounces on descriptor access,
and make future extensions easier, we put the ring itself at start of
page, and move the control after it.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Michael S. Tsirkin (2):
virtio: support layout with avail ring before idx
virtio: publish used idx
drivers/net/virtio_net.c | 2 ++
drivers/virtio/virtio_ring.c | 19 ++++++++++++++++---
include/linux/virtio_ring.h | 34 ++++++++++++++++++++++++++++------
3 files changed, 46 insertions(+), 9 deletions(-)
^ permalink raw reply
* [PATCHv2-RFC 1/2] virtio: support layout with avail ring before idx
From: Michael S. Tsirkin @ 2010-05-26 19:50 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
In-Reply-To: <cover.1274903328.git.mst@redhat.com>
This adds an (unused) option to put available ring
before control (avail index, flags). This avoids cache line
sharing between control and ring, and also
makes it possible to extend avail control without
incurring extra cache misses.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_ring.c | 4 ++--
include/linux/virtio_ring.h | 30 ++++++++++++++++++++++++------
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 1ca8890..2241342 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -224,7 +224,7 @@ add_head:
/* Put entry in available array (but don't update avail->idx until they
* do sync). FIXME: avoid modulus here? */
avail = (vq->vring.avail->idx + vq->num_added++) % vq->vring.num;
- vq->vring.avail->ring[avail] = head;
+ vq->vring.avail_ring[avail] = head;
pr_debug("Added buffer head %i to %p\n", head, vq);
END_USE(vq);
@@ -425,7 +425,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
if (!vq)
return NULL;
- vring_init(&vq->vring, num, pages, vring_align);
+ vring_init(&vq->vring, num, pages, vring_align, false);
vq->vq.callback = callback;
vq->vq.vdev = vdev;
vq->vq.name = name;
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index e4d144b..c5f3ee7 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -47,6 +47,11 @@ struct vring_avail {
__u16 ring[];
};
+struct vring_avail_ctrl {
+ __u16 flags;
+ __u16 idx;
+};
+
/* u32 is used here for ids for padding reasons. */
struct vring_used_elem {
/* Index of start of used descriptor chain. */
@@ -66,7 +71,9 @@ struct vring {
struct vring_desc *desc;
- struct vring_avail *avail;
+ struct vring_avail_ctrl *avail;
+
+ __u16 *avail_ring;
struct vring_used *used;
};
@@ -79,11 +86,18 @@ struct vring {
* // The actual descriptors (16 bytes each)
* struct vring_desc desc[num];
*
- * // A ring of available descriptor heads with free-running index.
+ * // A ring of available descriptor heads with a control structure
+ * // including a free-running index.
+ * // The ring can come either after (legacy) or before the control.
* __u16 avail_flags;
* __u16 avail_idx;
* __u16 available[num];
*
+ * or
+ *
+ * __u16 available[num];
+ * __u16 avail_flags;
+ * __u16 avail_idx;
* // Padding to the next align boundary.
* char pad[];
*
@@ -94,13 +108,17 @@ struct vring {
* };
*/
static inline void vring_init(struct vring *vr, unsigned int num, void *p,
- unsigned long align)
+ unsigned long align, bool avail_ring_first)
{
+ struct vring_avail *avail = p + num * sizeof(struct vring_desc);
vr->num = num;
vr->desc = p;
- vr->avail = p + num*sizeof(struct vring_desc);
- vr->used = (void *)(((unsigned long)&vr->avail->ring[num] + align-1)
- & ~(align - 1));
+ vr->avail_ring = avail_ring_first ? (void*)avail : &avail->ring;
+ vr->avail = avail_ring_first ? (void *)&vr->avail_ring[num] : p;
+ vr->used = (void *)ALIGN((unsigned long)&avail->ring[num], align);
+ /* Verify that avail fits before used. */
+ BUG_ON((unsigned long)(vr->avail + 1) > (unsigned long)vr->used);
+ BUG_ON((unsigned long)(&vr->avail_ring[num]) > (unsigned long)vr->used);
}
static inline unsigned vring_size(unsigned int num, unsigned long align)
--
1.7.1.12.g42b7f
^ permalink raw reply related
* [PATCHv2-RFC 2/2] virtio: publish used idx
From: Michael S. Tsirkin @ 2010-05-26 19:50 UTC (permalink / raw)
To: Rusty Russell, linux-kernel, virtualization, kvm, qemu-devel
In-Reply-To: <cover.1274903328.git.mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/virtio_net.c | 2 ++
drivers/virtio/virtio_ring.c | 17 +++++++++++++++--
include/linux/virtio_ring.h | 4 ++++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 78eb319..30e7483 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -23,6 +23,7 @@
#include <linux/module.h>
#include <linux/virtio.h>
#include <linux/virtio_net.h>
+#include <linux/virtio_ring.h>
#include <linux/scatterlist.h>
#include <linux/if_vlan.h>
#include <linux/slab.h>
@@ -1056,6 +1057,7 @@ static unsigned int features[] = {
VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
+ VIRTIO_RING_F_PUBLISH_USED,
};
static struct virtio_driver virtio_net_driver = {
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 2241342..4a5458e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -92,6 +92,9 @@ struct vring_virtqueue
/* Last used index we've seen. */
u16 last_used_idx;
+ /* Publish last used index we've seen at this location. */
+ u16 *publish_last_used_idx;
+
/* How to notify other side. FIXME: commonalize hcalls! */
void (*notify)(struct virtqueue *vq);
@@ -325,7 +328,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
/* detach_buf clears data, so grab it now. */
ret = vq->data[i];
detach_buf(vq, i);
- vq->last_used_idx++;
+ *vq->publish_last_used_idx = ++vq->last_used_idx;
END_USE(vq);
return ret;
}
@@ -348,6 +351,8 @@ bool virtqueue_enable_cb(struct virtqueue *_vq)
/* We optimistically turn back on interrupts, then check if there was
* more to do. */
vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
+ /* Besides flags write, this barrier also flushes out
+ * last available index write. */
virtio_mb();
if (unlikely(more_used(vq))) {
END_USE(vq);
@@ -425,13 +430,19 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
if (!vq)
return NULL;
- vring_init(&vq->vring, num, pages, vring_align, false);
+ vring_init(&vq->vring, num, pages, vring_align,
+ virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED));
+ if (virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_USED))
+ vq->publish_last_used_idx = &vq->vring.avail->last_used_idx;
+ else
+ vq->publish_last_used_idx = &vq->last_used_idx;
vq->vq.callback = callback;
vq->vq.vdev = vdev;
vq->vq.name = name;
vq->notify = notify;
vq->broken = false;
vq->last_used_idx = 0;
+ *vq->publish_last_used_idx = 0;
vq->num_added = 0;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
@@ -473,6 +484,8 @@ void vring_transport_features(struct virtio_device *vdev)
switch (i) {
case VIRTIO_RING_F_INDIRECT_DESC:
break;
+ case VIRTIO_RING_F_PUBLISH_USED:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev->features);
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index c5f3ee7..0de35c5 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -29,6 +29,9 @@
/* We support indirect buffer descriptors */
#define VIRTIO_RING_F_INDIRECT_DESC 28
+/* The Guest publishes last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_USED 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
struct vring_desc {
/* Address (guest-physical). */
@@ -50,6 +53,7 @@ struct vring_avail {
struct vring_avail_ctrl {
__u16 flags;
__u16 idx;
+ __u16 last_used_idx;
};
/* u32 is used here for ids for padding reasons. */
--
1.7.1.12.g42b7f
^ permalink raw reply related
* Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Greg KH @ 2010-05-26 20:51 UTC (permalink / raw)
To: Haiyang Zhang
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B8C4AC@TK5EX14MBXC128.redmond.corp.microsoft.com>
On Wed, May 26, 2010 at 04:54:39PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> Subject: [PATCH] staging: hv: Fix race condition on IC channel initialization
> There is a possible race condition when hv_utils starts to load immediately
> after hv_vmbus is loading - null pointer error could happen.
> This patch added an event waiting to ensure all channels are ready before
> vmbus_init() returns. So another module won't have any uninitialized channel.
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
>
> ---
> drivers/staging/hv/channel_mgmt.c | 23 +++++++++++++----------
> drivers/staging/hv/vmbus_drv.c | 10 ++++++++++
> drivers/staging/hv/vmbus_private.h | 1 +
> 3 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
> index 3f53b4d..f99db1b 100644
> --- a/drivers/staging/hv/channel_mgmt.c
> +++ b/drivers/staging/hv/channel_mgmt.c
> @@ -305,6 +305,7 @@ static void VmbusChannelProcessOffer(void *context)
> int ret;
> int cnt;
> unsigned long flags;
> + static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
Why is this an atomic_t?
> DPRINT_ENTER(VMBUS);
>
> @@ -373,22 +374,24 @@ static void VmbusChannelProcessOffer(void *context)
> * can cleanup properly
> */
> newChannel->State = CHANNEL_OPEN_STATE;
> - cnt = 0;
>
> - while (cnt != MAX_MSG_TYPES) {
> + /* Open IC channels */
> + for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
> if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
> &hv_cb_utils[cnt].data,
> - sizeof(struct hv_guid)) == 0) {
> + sizeof(struct hv_guid)) == 0 &&
> + VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> + 2 * PAGE_SIZE, NULL, 0,
> + hv_cb_utils[cnt].callback,
> + newChannel) == 0) {
> + hv_cb_utils[cnt].channel = newChannel;
> + mb();
What is the mb() call for? Why is it necessary? (hint, if you need it,
something else is really wrong...)
Something wierd happened with your indentation here, it doesn't line up
properly. That call to VmbusChannelOpen() needs to go in a full tab,
not 4 spaces.
Please always run your patch through the checkpatch.pl script before
sending it to me.
> DPRINT_INFO(VMBUS, "%s",
> hv_cb_utils[cnt].log_msg);
> -
> - if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> - 2 * PAGE_SIZE, NULL, 0,
> - hv_cb_utils[cnt].callback,
> - newChannel) == 0)
> - hv_cb_utils[cnt].channel = newChannel;
> + if (atomic_inc_return(&ic_channel_initcnt) ==
> + MAX_MSG_TYPES)
> + osd_WaitEventSet(ic_channel_ready);
> }
> - cnt++;
> }
> }
> DPRINT_EXIT(VMBUS);
> diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> index c21731a..3ae8981 100644
> --- a/drivers/staging/hv/vmbus_drv.c
> +++ b/drivers/staging/hv/vmbus_drv.c
> @@ -989,6 +989,8 @@ static struct dmi_system_id __initdata microsoft_hv_dmi_table[] = {
> };
> MODULE_DEVICE_TABLE(dmi, microsoft_hv_dmi_table);
>
> +struct osd_waitevent *ic_channel_ready;
What's with the "ic_" naming scheme here? It should be "hv_" right?
> +
> static int __init vmbus_init(void)
> {
> int ret = 0;
> @@ -1003,8 +1005,16 @@ static int __init vmbus_init(void)
> if (!dmi_check_system(microsoft_hv_dmi_table))
> return -ENODEV;
>
> + ic_channel_ready = osd_WaitEventCreate();
> + if (ic_channel_ready == NULL)
> + return -ENOMEM;
> +
> ret = vmbus_bus_init(VmbusInitialize);
As you are using this "ic_channel_ready variable only within the
vmbus_bus_init() call, why not just make it local to there? Then
there's no need to do the create/init/wait/free sequence outside the
init call.
The init call should just do all of this for us, right?
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Haiyang Zhang @ 2010-05-26 21:25 UTC (permalink / raw)
To: Greg KH
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <20100526205134.GC7343@suse.de>
> From: Greg KH [mailto:gregkh@suse.de]
> > + static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
> Why is this an atomic_t?
As discussed previously, I used atomic_t to handle more general case
if vmbus interrupts happen on every cpu.
> > + VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> > + 2 * PAGE_SIZE, NULL, 0,
> > + hv_cb_utils[cnt].callback,
> > + newChannel) == 0) {
> > + hv_cb_utils[cnt].channel = newChannel;
> > + mb();
>
> What is the mb() call for? Why is it necessary? (hint, if you need it,
> something else is really wrong...)
It ensures the channel assignment happens before the wakeup call:
osd_WaitEventSet(ic_channel_ready), if the compiler optimization re-arrange
the execution order.
> Something wierd happened with your indentation here, it doesn't line up
> properly. That call to VmbusChannelOpen() needs to go in a full tab,
> not 4 spaces.
>
> Please always run your patch through the checkpatch.pl script before
> sending it to me.
Sure, I will replace it with TAB. I already ran checkpatch.pl on
this patch -- no error:
staging-next-2.6> scripts/checkpatch.pl 0525-Fix-race-condition-on-IC-channel-initialization.patch
total: 0 errors, 0 warnings, 71 lines checked
0525-Fix-race-condition-on-IC-channel-initialization.patch has no obvious style problems and is ready for submission.
> > +struct osd_waitevent *ic_channel_ready;
>
> What's with the "ic_" naming scheme here? It should be "hv_" right?
IC stands for "integration components", such as Shutdown, Timesync,
Heartbeat, etc.
> As you are using this "ic_channel_ready variable only within the
> vmbus_bus_init() call, why not just make it local to there? Then
> there's no need to do the create/init/wait/free sequence outside the
> init call.
>
> The init call should just do all of this for us, right?
The ic_channel_ready variable is called by VmbusChannelProcessOffer / osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's
not a local variable.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Greg KH @ 2010-05-26 21:48 UTC (permalink / raw)
To: Haiyang Zhang
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B8C72B@TK5EX14MBXC128.redmond.corp.microsoft.com>
On Wed, May 26, 2010 at 09:25:31PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:gregkh@suse.de]
> > > + static atomic_t ic_channel_initcnt = ATOMIC_INIT(0);
> > Why is this an atomic_t?
>
> As discussed previously, I used atomic_t to handle more general case
> if vmbus interrupts happen on every cpu.
Ok, but then you should use a lock to protect the variable, not an atomic_t.
>
> > > + VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> > > + 2 * PAGE_SIZE, NULL, 0,
> > > + hv_cb_utils[cnt].callback,
> > > + newChannel) == 0) {
> > > + hv_cb_utils[cnt].channel = newChannel;
> > > + mb();
> >
> > What is the mb() call for? Why is it necessary? (hint, if you need it,
> > something else is really wrong...)
>
> It ensures the channel assignment happens before the wakeup call:
> osd_WaitEventSet(ic_channel_ready), if the compiler optimization re-arrange
> the execution order.
If you care about this, because some other thread is looking at it, then
you really need to protect it with a lock. Don't rely on a mb() to get
it all correct for you (hint, I doubt it will...)
> > Something wierd happened with your indentation here, it doesn't line up
> > properly. That call to VmbusChannelOpen() needs to go in a full tab,
> > not 4 spaces.
> >
> > Please always run your patch through the checkpatch.pl script before
> > sending it to me.
>
> Sure, I will replace it with TAB. I already ran checkpatch.pl on
> this patch -- no error:
> staging-next-2.6> scripts/checkpatch.pl 0525-Fix-race-condition-on-IC-channel-initialization.patch
> total: 0 errors, 0 warnings, 71 lines checked
>
> 0525-Fix-race-condition-on-IC-channel-initialization.patch has no obvious style problems and is ready for submission.
Looks like a bug in checkpatch.pl, go poke Andy about that please.
> > > +struct osd_waitevent *ic_channel_ready;
> >
> > What's with the "ic_" naming scheme here? It should be "hv_" right?
>
> IC stands for "integration components", such as Shutdown, Timesync,
> Heartbeat, etc.
Yes, I know what it stands for, but the rest of the world doesn't :)
> > As you are using this "ic_channel_ready variable only within the
> > vmbus_bus_init() call, why not just make it local to there? Then
> > there's no need to do the create/init/wait/free sequence outside the
> > init call.
> >
> > The init call should just do all of this for us, right?
>
> The ic_channel_ready variable is called by VmbusChannelProcessOffer /
> osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's
> not a local variable.
But again, this logic should be within the init call, as it's part of
the proper init sequence. So just put it in that call please.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Hank Janssen @ 2010-05-26 22:03 UTC (permalink / raw)
To: Greg KH, Haiyang Zhang
Cc: 'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org',
'linux-kernel@vger.kernel.org'
In-Reply-To: <20100526214842.GA27908@suse.de>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Wednesday, May 26, 2010 2:49 PM
> To: Haiyang Zhang
> Cc: 'linux-kernel@vger.kernel.org'; 'devel@driverdev.osuosl.org';
> 'virtualization@lists.osdl.org'; Hank Janssen
> Subject: Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel
> initialization (modified)
> On Wed, May 26, 2010 at 09:25:31PM +0000, Haiyang Zhang wrote:
>>> From: Greg KH [mailto:gregkh@suse.de]
>>> What's with the "ic_" naming scheme here? It should be "hv_"
> right?
>>
>> IC stands for "integration components", such as Shutdown, Timesync,
>> Heartbeat, etc.
>
> Yes, I know what it stands for, but the rest of the world doesn't :)
Marketing in the their infinite wisdom decided that going forward they
should be called Linux Integration Service so IS.
Sigh..............
We will change it to hv_ :)
Hank.
^ permalink raw reply
* RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Haiyang Zhang @ 2010-05-26 22:23 UTC (permalink / raw)
To: Greg KH
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <20100526214842.GA27908@suse.de>
> From: Greg KH [mailto:gregkh@suse.de]
> > As discussed previously, I used atomic_t to handle more general case
> > if vmbus interrupts happen on every cpu.
>
> Ok, but then you should use a lock to protect the variable, not an
> atomic_t.
> If you care about this, because some other thread is looking at it,
> then
> you really need to protect it with a lock. Don't rely on a mb() to get
> it all correct for you (hint, I doubt it will...)
Actually, since the interrupts only happen on cpu0, this is not a concern. How about use a simple int variable here? Also, remove the mb().
> > The ic_channel_ready variable is called by VmbusChannelProcessOffer /
> > osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's
> > not a local variable.
>
> But again, this logic should be within the init call, as it's part of
> the proper init sequence. So just put it in that call please.
The VmbusChannelProcessOffer() is called from interrupt context, and initialize the channels, wake up vmbus_init when all channels are ready. If using local variable only, how to pass the channel ready info to vmbus_init() which is in a different context?
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Greg KH @ 2010-05-26 22:30 UTC (permalink / raw)
To: Haiyang Zhang
Cc: 'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org',
'linux-kernel@vger.kernel.org'
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B8C775@TK5EX14MBXC128.redmond.corp.microsoft.com>
On Wed, May 26, 2010 at 10:23:08PM +0000, Haiyang Zhang wrote:
> > From: Greg KH [mailto:gregkh@suse.de]
> > > As discussed previously, I used atomic_t to handle more general case
> > > if vmbus interrupts happen on every cpu.
> >
> > Ok, but then you should use a lock to protect the variable, not an
> > atomic_t.
> > If you care about this, because some other thread is looking at it,
> > then
> > you really need to protect it with a lock. Don't rely on a mb() to get
> > it all correct for you (hint, I doubt it will...)
>
> Actually, since the interrupts only happen on cpu0, this is not a
> concern. How about use a simple int variable here? Also, remove the
> mb().
How about a lock!
What's so scary about a pretty little semaphore? They are all cute and
cuddley and don't bite anyone. You should not be afraid to use them,
they are here to do your bidding.
> > > The ic_channel_ready variable is called by VmbusChannelProcessOffer /
> > > osd_WaitEventSet(ic_channel_ready) to wake up vmbus_init(). So it's
> > > not a local variable.
> >
> > But again, this logic should be within the init call, as it's part of
> > the proper init sequence. So just put it in that call please.
>
> The VmbusChannelProcessOffer() is called from interrupt context, and
> initialize the channels, wake up vmbus_init when all channels are
> ready. If using local variable only, how to pass the channel ready
> info to vmbus_init() which is in a different context?
No, I mean move the logic you added here, into the vmbus_init() call.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Haiyang Zhang @ 2010-05-26 22:52 UTC (permalink / raw)
To: Greg KH
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <20100526223053.GA23521@suse.de>
> From: Greg KH [mailto:gregkh@suse.de]
> How about a lock!
>
> What's so scary about a pretty little semaphore? They are all cute and
> cuddley and don't bite anyone. You should not be afraid to use them,
> they are here to do your bidding.
No problem, we will add a lock here.
> > The VmbusChannelProcessOffer() is called from interrupt context, and
> > initialize the channels, wake up vmbus_init when all channels are
> > ready. If using local variable only, how to pass the channel ready
> > info to vmbus_init() which is in a different context?
>
> No, I mean move the logic you added here, into the vmbus_init() call.
Do you mean:
Move the event creat/wait/free, which is currently in vmbus_init(), into vmbus_bus_init() function. hv_channle_ready will still be a global variable. And, the wakeup call -- osd_WaitEventSet() --remains in VmbusChannelProcessOffer() ?
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH 1/1] staging: hv: Fix race condition on IC channel initialization (modified)
From: Jiri Slaby @ 2010-05-27 6:16 UTC (permalink / raw)
To: Haiyang Zhang
Cc: Greg KH, 'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org',
'linux-kernel@vger.kernel.org'
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B8C72B@TK5EX14MBXC128.redmond.corp.microsoft.com>
On 05/26/2010 11:25 PM, Haiyang Zhang wrote:
>> From: Greg KH [mailto:gregkh@suse.de]
>>> + VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
>>> + 2 * PAGE_SIZE, NULL, 0,
>>> + hv_cb_utils[cnt].callback,
>>> + newChannel) == 0) {
>>> + hv_cb_utils[cnt].channel = newChannel;
>>> + mb();
>>
>> What is the mb() call for? Why is it necessary? (hint, if you need it,
>> something else is really wrong...)
>
> It ensures the channel assignment happens before the wakeup call:
> osd_WaitEventSet(ic_channel_ready), if the compiler optimization re-arrange
> the execution order.
wake_up() is a barrier, you don't need the mb() there.
BTW osd_WaitEventSet et al. can be easily converted to completion.
--
js
^ permalink raw reply
* [PATCH 1/2] virtio: console: Fix crash when hot-unplugging a port and read is blocked
From: Amit Shah @ 2010-05-27 7:54 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Virtualization List
When removing a port we don't check if a program was blocked for read.
This leads to a crash when SIGTERM is sent to the program after
hot-unplugging the port.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 10 +++++++---
1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8c99bf1..e3fb529 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1099,6 +1099,13 @@ static int remove_port(struct port *port)
{
struct port_buffer *buf;
+ if (port->guest_connected) {
+ port->guest_connected = false;
+ port->host_connected = false;
+ wake_up_interruptible(&port->waitqueue);
+ send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
+ }
+
spin_lock_irq(&port->portdev->ports_lock);
list_del(&port->list);
spin_unlock_irq(&port->portdev->ports_lock);
@@ -1120,9 +1127,6 @@ static int remove_port(struct port *port)
hvc_remove(port->cons.hvc);
#endif
}
- if (port->guest_connected)
- send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
-
sysfs_remove_group(&port->dev->kobj, &port_attribute_group);
device_destroy(pdrvdata.class, port->dev->devt);
cdev_del(&port->cdev);
--
1.7.0.1
^ permalink raw reply related
* [PATCH 2/2] virtio: console: Fix crash when port is unplugged and blocked for write
From: Amit Shah @ 2010-05-27 7:54 UTC (permalink / raw)
To: Rusty Russell; +Cc: Amit Shah, Virtualization List
In-Reply-To: <712d1fa57175fb900ca6ebb133849497f4c2448b.1274946878.git.amit.shah@redhat.com>
When a program that has a virtio port opened and blocked for a write
operation, a port hot-unplug event will later led to a crash when
SIGTERM was sent to the program. Fix that.
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
drivers/char/virtio_console.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e3fb529..942a982 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -529,6 +529,10 @@ static bool will_write_block(struct port *port)
{
bool ret;
+ if (!port->guest_connected) {
+ /* Port got hot-unplugged. Let's exit. */
+ return false;
+ }
if (!port->host_connected)
return true;
--
1.7.0.1
^ permalink raw reply related
* [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly
From: Takuya Yoshikawa @ 2010-05-27 9:58 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev
copy_to/from_user() returns the number of bytes that could not be copied.
So we need to check if it is not zero, and in that case, we should return
the error number -EFAULT rather than directly return the return value from
copy_to/from_user().
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
drivers/vhost/vhost.c | 51 ++++++++++++++++++++++++++----------------------
1 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5c9c657..9633a3c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -320,10 +320,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
{
struct vhost_memory mem, *newmem, *oldmem;
unsigned long size = offsetof(struct vhost_memory, regions);
- long r;
- r = copy_from_user(&mem, m, size);
- if (r)
- return r;
+ if (copy_from_user(&mem, m, size))
+ return -EFAULT;
if (mem.padding)
return -EOPNOTSUPP;
if (mem.nregions > VHOST_MEMORY_MAX_NREGIONS)
@@ -333,11 +331,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
return -ENOMEM;
memcpy(newmem, &mem, size);
- r = copy_from_user(newmem->regions, m->regions,
- mem.nregions * sizeof *m->regions);
- if (r) {
+ if (copy_from_user(newmem->regions, m->regions,
+ mem.nregions * sizeof *m->regions)) {
kfree(newmem);
- return r;
+ return -EFAULT;
}
if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL)))
@@ -389,9 +386,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
r = -EBUSY;
break;
}
- r = copy_from_user(&s, argp, sizeof s);
- if (r < 0)
+ if (copy_from_user(&s, argp, sizeof s)) {
+ r = -EFAULT;
break;
+ }
if (!s.num || s.num > 0xffff || (s.num & (s.num - 1))) {
r = -EINVAL;
break;
@@ -405,9 +403,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
r = -EBUSY;
break;
}
- r = copy_from_user(&s, argp, sizeof s);
- if (r < 0)
+ if (copy_from_user(&s, argp, sizeof s)) {
+ r = -EFAULT;
break;
+ }
if (s.num > 0xffff) {
r = -EINVAL;
break;
@@ -419,12 +418,14 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
case VHOST_GET_VRING_BASE:
s.index = idx;
s.num = vq->last_avail_idx;
- r = copy_to_user(argp, &s, sizeof s);
+ if (copy_to_user(argp, &s, sizeof s))
+ r = -EFAULT;
break;
case VHOST_SET_VRING_ADDR:
- r = copy_from_user(&a, argp, sizeof a);
- if (r < 0)
+ if (copy_from_user(&a, argp, sizeof a)) {
+ r = -EFAULT;
break;
+ }
if (a.flags & ~(0x1 << VHOST_VRING_F_LOG)) {
r = -EOPNOTSUPP;
break;
@@ -477,9 +478,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
vq->used = (void __user *)(unsigned long)a.used_user_addr;
break;
case VHOST_SET_VRING_KICK:
- r = copy_from_user(&f, argp, sizeof f);
- if (r < 0)
+ if (copy_from_user(&f, argp, sizeof f)) {
+ r = -EFAULT;
break;
+ }
eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
if (IS_ERR(eventfp)) {
r = PTR_ERR(eventfp);
@@ -492,9 +494,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
filep = eventfp;
break;
case VHOST_SET_VRING_CALL:
- r = copy_from_user(&f, argp, sizeof f);
- if (r < 0)
+ if (copy_from_user(&f, argp, sizeof f)) {
+ r = -EFAULT;
break;
+ }
eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
if (IS_ERR(eventfp)) {
r = PTR_ERR(eventfp);
@@ -510,9 +513,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
filep = eventfp;
break;
case VHOST_SET_VRING_ERR:
- r = copy_from_user(&f, argp, sizeof f);
- if (r < 0)
+ if (copy_from_user(&f, argp, sizeof f)) {
+ r = -EFAULT;
break;
+ }
eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
if (IS_ERR(eventfp)) {
r = PTR_ERR(eventfp);
@@ -575,9 +579,10 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
r = vhost_set_memory(d, argp);
break;
case VHOST_SET_LOG_BASE:
- r = copy_from_user(&p, argp, sizeof p);
- if (r < 0)
+ if (copy_from_user(&p, argp, sizeof p)) {
+ r = -EFAULT;
break;
+ }
if ((u64)(unsigned long)p != p) {
r = -EFAULT;
break;
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly
From: Takuya Yoshikawa @ 2010-05-27 10:01 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev
In-Reply-To: <20100527185803.0c0213a1.yoshikawa.takuya@oss.ntt.co.jp>
copy_to/from_user() returns the number of bytes that could not be copied.
So we need to check if it is not zero, and in that case, we should return
the error number -EFAULT rather than directly return the return value from
copy_to/from_user().
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
drivers/vhost/net.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index aa88911..0f41c91 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -593,17 +593,17 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
int r;
switch (ioctl) {
case VHOST_NET_SET_BACKEND:
- r = copy_from_user(&backend, argp, sizeof backend);
- if (r < 0)
- return r;
+ if (copy_from_user(&backend, argp, sizeof backend))
+ return -EFAULT;
return vhost_net_set_backend(n, backend.index, backend.fd);
case VHOST_GET_FEATURES:
features = VHOST_FEATURES;
- return copy_to_user(featurep, &features, sizeof features);
+ if (copy_to_user(featurep, &features, sizeof features))
+ return -EFAULT;
+ return 0;
case VHOST_SET_FEATURES:
- r = copy_from_user(&features, featurep, sizeof features);
- if (r < 0)
- return r;
+ if (copy_from_user(&features, featurep, sizeof features))
+ return -EFAULT;
if (features & ~VHOST_FEATURES)
return -EOPNOTSUPP;
return vhost_net_set_features(n, features);
--
1.7.0.4
^ permalink raw reply related
* [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails
From: Takuya Yoshikawa @ 2010-05-27 10:03 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev
In-Reply-To: <20100527185803.0c0213a1.yoshikawa.takuya@oss.ntt.co.jp>
We need to free newmem when vhost_set_memory() fails to complete.
Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
drivers/vhost/vhost.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9633a3c..1241a22 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -337,8 +337,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
return -EFAULT;
}
- if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL)))
+ if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) {
+ kfree(newmem);
return -EFAULT;
+ }
oldmem = d->memory;
rcu_assign_pointer(d->memory, newmem);
synchronize_rcu();
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 1/3] vhost: fix to check the return value of copy_to/from_user() correctly
From: Michael S. Tsirkin @ 2010-05-27 10:48 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: kvm, virtualization, netdev
In-Reply-To: <20100527185803.0c0213a1.yoshikawa.takuya@oss.ntt.co.jp>
On Thu, May 27, 2010 at 06:58:03PM +0900, Takuya Yoshikawa wrote:
> copy_to/from_user() returns the number of bytes that could not be copied.
>
> So we need to check if it is not zero, and in that case, we should return
> the error number -EFAULT rather than directly return the return value from
> copy_to/from_user().
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Thanks, applied.
> ---
> drivers/vhost/vhost.c | 51 ++++++++++++++++++++++++++----------------------
> 1 files changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 5c9c657..9633a3c 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -320,10 +320,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> {
> struct vhost_memory mem, *newmem, *oldmem;
> unsigned long size = offsetof(struct vhost_memory, regions);
> - long r;
> - r = copy_from_user(&mem, m, size);
> - if (r)
> - return r;
> + if (copy_from_user(&mem, m, size))
> + return -EFAULT;
> if (mem.padding)
> return -EOPNOTSUPP;
> if (mem.nregions > VHOST_MEMORY_MAX_NREGIONS)
> @@ -333,11 +331,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> return -ENOMEM;
>
> memcpy(newmem, &mem, size);
> - r = copy_from_user(newmem->regions, m->regions,
> - mem.nregions * sizeof *m->regions);
> - if (r) {
> + if (copy_from_user(newmem->regions, m->regions,
> + mem.nregions * sizeof *m->regions)) {
> kfree(newmem);
> - return r;
> + return -EFAULT;
> }
>
> if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL)))
> @@ -389,9 +386,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> r = -EBUSY;
> break;
> }
> - r = copy_from_user(&s, argp, sizeof s);
> - if (r < 0)
> + if (copy_from_user(&s, argp, sizeof s)) {
> + r = -EFAULT;
> break;
> + }
> if (!s.num || s.num > 0xffff || (s.num & (s.num - 1))) {
> r = -EINVAL;
> break;
> @@ -405,9 +403,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> r = -EBUSY;
> break;
> }
> - r = copy_from_user(&s, argp, sizeof s);
> - if (r < 0)
> + if (copy_from_user(&s, argp, sizeof s)) {
> + r = -EFAULT;
> break;
> + }
> if (s.num > 0xffff) {
> r = -EINVAL;
> break;
> @@ -419,12 +418,14 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> case VHOST_GET_VRING_BASE:
> s.index = idx;
> s.num = vq->last_avail_idx;
> - r = copy_to_user(argp, &s, sizeof s);
> + if (copy_to_user(argp, &s, sizeof s))
> + r = -EFAULT;
> break;
> case VHOST_SET_VRING_ADDR:
> - r = copy_from_user(&a, argp, sizeof a);
> - if (r < 0)
> + if (copy_from_user(&a, argp, sizeof a)) {
> + r = -EFAULT;
> break;
> + }
> if (a.flags & ~(0x1 << VHOST_VRING_F_LOG)) {
> r = -EOPNOTSUPP;
> break;
> @@ -477,9 +478,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> vq->used = (void __user *)(unsigned long)a.used_user_addr;
> break;
> case VHOST_SET_VRING_KICK:
> - r = copy_from_user(&f, argp, sizeof f);
> - if (r < 0)
> + if (copy_from_user(&f, argp, sizeof f)) {
> + r = -EFAULT;
> break;
> + }
> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
> if (IS_ERR(eventfp)) {
> r = PTR_ERR(eventfp);
> @@ -492,9 +494,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> filep = eventfp;
> break;
> case VHOST_SET_VRING_CALL:
> - r = copy_from_user(&f, argp, sizeof f);
> - if (r < 0)
> + if (copy_from_user(&f, argp, sizeof f)) {
> + r = -EFAULT;
> break;
> + }
> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
> if (IS_ERR(eventfp)) {
> r = PTR_ERR(eventfp);
> @@ -510,9 +513,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
> filep = eventfp;
> break;
> case VHOST_SET_VRING_ERR:
> - r = copy_from_user(&f, argp, sizeof f);
> - if (r < 0)
> + if (copy_from_user(&f, argp, sizeof f)) {
> + r = -EFAULT;
> break;
> + }
> eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
> if (IS_ERR(eventfp)) {
> r = PTR_ERR(eventfp);
> @@ -575,9 +579,10 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
> r = vhost_set_memory(d, argp);
> break;
> case VHOST_SET_LOG_BASE:
> - r = copy_from_user(&p, argp, sizeof p);
> - if (r < 0)
> + if (copy_from_user(&p, argp, sizeof p)) {
> + r = -EFAULT;
> break;
> + }
> if ((u64)(unsigned long)p != p) {
> r = -EFAULT;
> break;
> --
> 1.7.0.4
^ permalink raw reply
* Re: [PATCH 2/3] vhost-net: fix to check the return value of copy_to/from_user() correctly
From: Michael S. Tsirkin @ 2010-05-27 10:48 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: kvm, virtualization, netdev
In-Reply-To: <20100527190158.4587a2db.yoshikawa.takuya@oss.ntt.co.jp>
On Thu, May 27, 2010 at 07:01:58PM +0900, Takuya Yoshikawa wrote:
> copy_to/from_user() returns the number of bytes that could not be copied.
>
> So we need to check if it is not zero, and in that case, we should return
> the error number -EFAULT rather than directly return the return value from
> copy_to/from_user().
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Thanks, applied.
> ---
> drivers/vhost/net.c | 14 +++++++-------
> 1 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index aa88911..0f41c91 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -593,17 +593,17 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> int r;
> switch (ioctl) {
> case VHOST_NET_SET_BACKEND:
> - r = copy_from_user(&backend, argp, sizeof backend);
> - if (r < 0)
> - return r;
> + if (copy_from_user(&backend, argp, sizeof backend))
> + return -EFAULT;
> return vhost_net_set_backend(n, backend.index, backend.fd);
> case VHOST_GET_FEATURES:
> features = VHOST_FEATURES;
> - return copy_to_user(featurep, &features, sizeof features);
> + if (copy_to_user(featurep, &features, sizeof features))
> + return -EFAULT;
> + return 0;
> case VHOST_SET_FEATURES:
> - r = copy_from_user(&features, featurep, sizeof features);
> - if (r < 0)
> - return r;
> + if (copy_from_user(&features, featurep, sizeof features))
> + return -EFAULT;
> if (features & ~VHOST_FEATURES)
> return -EOPNOTSUPP;
> return vhost_net_set_features(n, features);
> --
> 1.7.0.4
^ permalink raw reply
* Re: [PATCH 3/3] vhost: fix the memory leak which will happen when memory_access_ok fails
From: Michael S. Tsirkin @ 2010-05-27 10:49 UTC (permalink / raw)
To: Takuya Yoshikawa; +Cc: kvm, virtualization, netdev
In-Reply-To: <20100527190356.cbf2aac7.yoshikawa.takuya@oss.ntt.co.jp>
On Thu, May 27, 2010 at 07:03:56PM +0900, Takuya Yoshikawa wrote:
> We need to free newmem when vhost_set_memory() fails to complete.
>
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> ---
Thanks, applied.
> drivers/vhost/vhost.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 9633a3c..1241a22 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -337,8 +337,10 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> return -EFAULT;
> }
>
> - if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL)))
> + if (!memory_access_ok(d, newmem, vhost_has_feature(d, VHOST_F_LOG_ALL))) {
> + kfree(newmem);
> return -EFAULT;
> + }
> oldmem = d->memory;
> rcu_assign_pointer(d->memory, newmem);
> synchronize_rcu();
> --
> 1.7.0.4
^ permalink raw reply
* [GIT PULL net-next-2.6] vhost-net cleanups
From: Michael S. Tsirkin @ 2010-05-27 10:58 UTC (permalink / raw)
To: David Miller; +Cc: kvm, virtualization, netdev, linux-kernel, jdike, tklauser
David,
The following tree is on top of net-next-2.6.
Please merge it for 2.6.36.
Thanks!
The following changes since commit 7a9b149212f3716c598afe973b6261fd58453b7a:
Merge git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb-2.6 (2010-05-20 21:26:12 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next
Jeff Dike (1):
vhost-net: minor cleanup
Michael S. Tsirkin (1):
vhost: whitespace fix
Tobias Klauser (1):
vhost: Storage class should be before const qualifier
drivers/vhost/net.c | 13 ++++++-------
drivers/vhost/vhost.c | 4 ++--
2 files changed, 8 insertions(+), 9 deletions(-)
--
MST
^ permalink raw reply
* [GIT PULL net-2.6] vhost-net: error handling fixes
From: Michael S. Tsirkin @ 2010-05-27 11:57 UTC (permalink / raw)
To: David Miller; +Cc: kvm, virtualization, netdev, linux-kernel, krkumar2
David,
The following tree includes fixes dealing with error handling
in vhost-net. It is on top of net-2.6.
Please merge it for 2.6.35.
Thanks!
The following changes since commit 8a74ad60a546b13bd1096b2a61a7a5c6fd9ae17c:
net: fix lock_sock_bh/unlock_sock_bh (2010-05-27 00:30:53 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net
Krishna Kumar (1):
vhost: Fix host panic if ioctl called with wrong index
Takuya Yoshikawa (3):
vhost: fix to check the return value of copy_to/from_user() correctly
vhost-net: fix to check the return value of copy_to/from_user() correctly
vhost: fix the memory leak which will happen when memory_access_ok fails
drivers/vhost/net.c | 14 ++++++------
drivers/vhost/vhost.c | 57 +++++++++++++++++++++++++++---------------------
2 files changed, 39 insertions(+), 32 deletions(-)
--
MST
^ 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