* [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages @ 2012-10-17 10:14 Will Deacon 2012-10-17 16:05 ` Sasha Levin 2012-10-18 2:19 ` Rusty Russell 0 siblings, 2 replies; 6+ messages in thread From: Will Deacon @ 2012-10-17 10:14 UTC (permalink / raw) To: linux-kernel; +Cc: Will Deacon, Rusty Russell, Sasha Levin, Marc Zyngier When using a virtio transport, the 9p net device allocates pages to back the descriptors inserted into the virtqueue. These allocations may be performed from atomic context (under the channel lock) and can therefore return high mappings which aren't suitable for virt_to_phys. This patch ensures that virtual addresses for virtio queue descriptors are converted to physical addresses using kmap_to_page, which handles high memory correctly. Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Sasha Levin <levinsasha928@gmail.com> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- drivers/virtio/virtio_ring.c | 4 +++- net/9p/trans_virtio.c | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index e639584..be93946 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -22,6 +22,7 @@ #include <linux/device.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/highmem.h> #include <linux/hrtimer.h> /* virtio guest is communicating with a virtual "device" that actually runs on @@ -165,7 +166,8 @@ static int vring_add_indirect(struct vring_virtqueue *vq, /* Use a single buffer which doesn't continue */ head = vq->free_head; vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT; - vq->vring.desc[head].addr = virt_to_phys(desc); + vq->vring.desc[head].addr = page_to_phys(kmap_to_page(desc)) + + ((unsigned long)desc & ~PAGE_MASK); vq->vring.desc[head].len = i * sizeof(struct vring_desc); /* Update free pointer */ diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index 35b8911..fd05c81 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -39,6 +39,7 @@ #include <linux/inet.h> #include <linux/idr.h> #include <linux/file.h> +#include <linux/highmem.h> #include <linux/slab.h> #include <net/9p/9p.h> #include <linux/parser.h> @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan, int count = nr_pages; while (nr_pages) { s = rest_of_page(data); - pages[index++] = virt_to_page(data); + pages[index++] = kmap_to_page(data); data += s; nr_pages--; } -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages 2012-10-17 10:14 [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages Will Deacon @ 2012-10-17 16:05 ` Sasha Levin 2012-10-17 16:17 ` Will Deacon 2012-10-18 2:19 ` Rusty Russell 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2012-10-17 16:05 UTC (permalink / raw) To: Will Deacon; +Cc: linux-kernel, Rusty Russell, Marc Zyngier On Wed, Oct 17, 2012 at 6:14 AM, Will Deacon <will.deacon@arm.com> wrote: > When using a virtio transport, the 9p net device allocates pages to back > the descriptors inserted into the virtqueue. These allocations may be > performed from atomic context (under the channel lock) and can therefore > return high mappings which aren't suitable for virt_to_phys. > > This patch ensures that virtual addresses for virtio queue descriptors > are converted to physical addresses using kmap_to_page, which handles > high memory correctly. > > Cc: Rusty Russell <rusty@rustcorp.com.au> > Cc: Sasha Levin <levinsasha928@gmail.com> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- One of the things I was seeing when fuzzing within lkvm were random crashes resulting from "odd" addresses used by virtio-9p queue descriptions. I attributed that to memory corruption as a result of something else failing the fuzz tests before, but this patch explains what might have gone wrong. I'll plug it in and run tests on it. Thanks, Sasha ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages 2012-10-17 16:05 ` Sasha Levin @ 2012-10-17 16:17 ` Will Deacon 0 siblings, 0 replies; 6+ messages in thread From: Will Deacon @ 2012-10-17 16:17 UTC (permalink / raw) To: Sasha Levin; +Cc: linux-kernel@vger.kernel.org, Rusty Russell, Marc Zyngier On Wed, Oct 17, 2012 at 05:05:00PM +0100, Sasha Levin wrote: > On Wed, Oct 17, 2012 at 6:14 AM, Will Deacon <will.deacon@arm.com> wrote: > > When using a virtio transport, the 9p net device allocates pages to back > > the descriptors inserted into the virtqueue. These allocations may be > > performed from atomic context (under the channel lock) and can therefore > > return high mappings which aren't suitable for virt_to_phys. > > > > This patch ensures that virtual addresses for virtio queue descriptors > > are converted to physical addresses using kmap_to_page, which handles > > high memory correctly. > > > > Cc: Rusty Russell <rusty@rustcorp.com.au> > > Cc: Sasha Levin <levinsasha928@gmail.com> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > --- > > One of the things I was seeing when fuzzing within lkvm were random > crashes resulting > from "odd" addresses used by virtio-9p queue descriptions. I > attributed that to memory > corruption as a result of something else failing the fuzz tests > before, but this patch > explains what might have gone wrong. This is *exactly* what I have observed. More specifically, I hacked kvmtool to support memory banks above 0x0 (I'll send some patches soon) so my PHYS_OFFSET was 0x80000000. virtio-9p then gave me physical addresses at 0x7fe02000, which if linearly mapped would be 0xffe02000 -- the pkmap! > I'll plug it in and run tests on it. The kernel-janitors list pointed out to me that we need to EXPORT_SYMBOL on kmap_to_page, so if you're building the virtio ring as a module you'll need to do that. Cheers, Will ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages 2012-10-17 10:14 [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages Will Deacon 2012-10-17 16:05 ` Sasha Levin @ 2012-10-18 2:19 ` Rusty Russell 2012-10-18 9:42 ` Will Deacon 1 sibling, 1 reply; 6+ messages in thread From: Rusty Russell @ 2012-10-18 2:19 UTC (permalink / raw) To: Will Deacon, linux-kernel Cc: Will Deacon, Sasha Levin, Marc Zyngier, lf-virt, Andrew Morton, Eric Van Hensbergen Will Deacon <will.deacon@arm.com> writes: > When using a virtio transport, the 9p net device allocates pages to back > the descriptors inserted into the virtqueue. These allocations may be > performed from atomic context (under the channel lock) and can therefore > return high mappings which aren't suitable for virt_to_phys. I had not appreciated that subtlety about GFP_ATOMIC :( This isn't just 9p, the console, block, scsi and net devices also use GFP_ATOMIC. > @@ -165,7 +166,8 @@ static int vring_add_indirect(struct vring_virtqueue *vq, > /* Use a single buffer which doesn't continue */ > head = vq->free_head; > vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT; > - vq->vring.desc[head].addr = virt_to_phys(desc); > + vq->vring.desc[head].addr = page_to_phys(kmap_to_page(desc)) + > + ((unsigned long)desc & ~PAGE_MASK); > vq->vring.desc[head].len = i * sizeof(struct vring_desc); Gah, virt_to_phys_harder()? What's the performance effect? If it's negligible, why doesn't virt_to_phys() just do this for us? We do have an alternate solution: masking out __GFP_HIGHMEM from the kmalloc of desc. If it fails, we will fall back to laying out the virtio request directly inside the ring; if it doesn't fit, we'll wait for the device to consume more buffers. > @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan, > int count = nr_pages; > while (nr_pages) { > s = rest_of_page(data); > - pages[index++] = virt_to_page(data); > + pages[index++] = kmap_to_page(data); > data += s; > nr_pages--; > } This seems like a separate bug fix. Cheers, Rusty. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages 2012-10-18 2:19 ` Rusty Russell @ 2012-10-18 9:42 ` Will Deacon 2012-10-18 23:39 ` Rusty Russell 0 siblings, 1 reply; 6+ messages in thread From: Will Deacon @ 2012-10-18 9:42 UTC (permalink / raw) To: Rusty Russell Cc: linux-kernel@vger.kernel.org, Sasha Levin, Marc Zyngier, lf-virt, Andrew Morton, Eric Van Hensbergen Hi Rusty, On Thu, Oct 18, 2012 at 03:19:06AM +0100, Rusty Russell wrote: > Will Deacon <will.deacon@arm.com> writes: > > When using a virtio transport, the 9p net device allocates pages to back > > the descriptors inserted into the virtqueue. These allocations may be > > performed from atomic context (under the channel lock) and can therefore > > return high mappings which aren't suitable for virt_to_phys. > > I had not appreciated that subtlety about GFP_ATOMIC :( Yeah, it's unfortunate for poor old userspace. > This isn't just 9p, the console, block, scsi and net devices also use > GFP_ATOMIC. Ok, I'll split this patch in two since I think that only 9p has the zero-copy stuff, which is why an extra fix is needed there for creating the scatterlist correctly. > > @@ -165,7 +166,8 @@ static int vring_add_indirect(struct vring_virtqueue *vq, > > /* Use a single buffer which doesn't continue */ > > head = vq->free_head; > > vq->vring.desc[head].flags = VRING_DESC_F_INDIRECT; > > - vq->vring.desc[head].addr = virt_to_phys(desc); > > + vq->vring.desc[head].addr = page_to_phys(kmap_to_page(desc)) + > > + ((unsigned long)desc & ~PAGE_MASK); > > vq->vring.desc[head].len = i * sizeof(struct vring_desc); > > Gah, virt_to_phys_harder()? Tell me about it... > What's the performance effect? If it's negligible, why doesn't > virt_to_phys() just do this for us? I've not measured it, but even when you don't have CONFIG_HIGHMEM, there's going to be an overhead here because we go around the houses to get the page and then add the offset on afterwards. I doubt it's something we want to plumb directly into virt_to_phys (also, kmap_to_page may call virt_to_phys via the __pa macro so we'd get stuck). > We do have an alternate solution: masking out __GFP_HIGHMEM from the > kmalloc of desc. If it fails, we will fall back to laying out the > virtio request directly inside the ring; if it doesn't fit, we'll wait > for the device to consume more buffers. Hmm, that will probably work for the vring but the zero-copy code for 9p may just give us an address from userspace if I'm understanding it correctly. In that case, we really have to do the translation as below (which is actually much cleaner because everything is page-aligned). > > @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan, > > int count = nr_pages; > > while (nr_pages) { > > s = rest_of_page(data); > > - pages[index++] = virt_to_page(data); > > + pages[index++] = kmap_to_page(data); > > data += s; > > nr_pages--; > > } So what do you reckon? How about I leave this hunk as a separate patch and have a play masking out __GFP_HIGHMEM for the vring descriptor? Cheers, Will ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages 2012-10-18 9:42 ` Will Deacon @ 2012-10-18 23:39 ` Rusty Russell 0 siblings, 0 replies; 6+ messages in thread From: Rusty Russell @ 2012-10-18 23:39 UTC (permalink / raw) To: Will Deacon Cc: linux-kernel@vger.kernel.org, Sasha Levin, Marc Zyngier, lf-virt, Andrew Morton, Eric Van Hensbergen Will Deacon <will.deacon@arm.com> writes: > On Thu, Oct 18, 2012 at 03:19:06AM +0100, Rusty Russell wrote: >> We do have an alternate solution: masking out __GFP_HIGHMEM from the >> kmalloc of desc. If it fails, we will fall back to laying out the >> virtio request directly inside the ring; if it doesn't fit, we'll wait >> for the device to consume more buffers. > > Hmm, that will probably work for the vring but the zero-copy code for 9p may > just give us an address from userspace if I'm understanding it correctly. In > that case, we really have to do the translation as below (which is actually > much cleaner because everything is page-aligned). > >> > @@ -325,7 +326,7 @@ static int p9_get_mapped_pages(struct virtio_chan *chan, >> > int count = nr_pages; >> > while (nr_pages) { >> > s = rest_of_page(data); >> > - pages[index++] = virt_to_page(data); >> > + pages[index++] = kmap_to_page(data); >> > data += s; >> > nr_pages--; >> > } > > So what do you reckon? How about I leave this hunk as a separate patch and > have a play masking out __GFP_HIGHMEM for the vring descriptor? Yes, I think so. A scathing comment would be nice, too... Thanks, Rusty. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-19 0:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-17 10:14 [PATCH] virtio: 9p: correctly pass physical address to userspace for high pages Will Deacon 2012-10-17 16:05 ` Sasha Levin 2012-10-17 16:17 ` Will Deacon 2012-10-18 2:19 ` Rusty Russell 2012-10-18 9:42 ` Will Deacon 2012-10-18 23:39 ` Rusty Russell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox