[PATCH] 9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers (second submission)

[PATCH] 9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers (second submission)

[Richard Yao]

This is my first real patch submission, which I sent out in December.
Unfortunately, I sent it to only a subset of the correct people. Time passed,
Greg Koah-Hartman was kind enough to give me some pointers and I am now sending
it back out to the correct people. My apologies to anyone who was bit by this
issue because it was not sent to the correct people sooner.

Richard Yao (1):
  9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers

 net/9p/trans_virtio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
1.8.3.2

[PATCH] 9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers

[Richard Yao]

The 9p-virtio transport does zero copy on things larger than 1024 bytes
in size. It accomplishes this by returning the physical addresses of
pages to the virtio-pci device. At present, the translation is usually a
bit shift.

However, that approach produces an invalid page address when we
read/write to vmalloc buffers, such as those used for Linux kernle
modules. This causes QEMU to die printing:

qemu-system-x86_64: virtio: trying to map MMIO memory

This patch enables 9p-virtio to correctly handle this case. This not
only enables us to load Linux kernel modules off virtfs, but also
enables ZFS file-based vdevs on virtfs to be used without killing QEMU.

Also, special thanks to both Avi Kivity and Alexander Graf for their
interpretation of QEMU backtraces. Without their guidence, tracking down
this bug would have taken much longer.

Signed-off-by: Richard Yao <ryao@gentoo.org>
Acked-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Will Deacon <will.deacon@arm.com>
---
 net/9p/trans_virtio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index cd1e1ed..b2009bc 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -340,7 +340,10 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
 		int count = nr_pages;
 		while (nr_pages) {
 			s = rest_of_page(data);
-			pages[index++] = kmap_to_page(data);
+			if (is_vmalloc_or_module_addr(data))
+				pages[index++] = vmalloc_to_page(data);
+			else
+				pages[index++] = kmap_to_page(data);
 			data += s;
 			nr_pages--;
 		}
-- 
1.8.3.2

Re: [PATCH] 9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers

[David Miller]

From: Richard Yao <ryao@gentoo.org>
Date: Thu, 30 Jan 2014 13:02:48 -0500

> The 9p-virtio transport does zero copy on things larger than 1024 bytes
> in size. It accomplishes this by returning the physical addresses of
> pages to the virtio-pci device. At present, the translation is usually a
> bit shift.
> 
> However, that approach produces an invalid page address when we
> read/write to vmalloc buffers, such as those used for Linux kernle
> modules. This causes QEMU to die printing:
> 
> qemu-system-x86_64: virtio: trying to map MMIO memory
> 
> This patch enables 9p-virtio to correctly handle this case. This not
> only enables us to load Linux kernel modules off virtfs, but also
> enables ZFS file-based vdevs on virtfs to be used without killing QEMU.
> 
> Also, special thanks to both Avi Kivity and Alexander Graf for their
> interpretation of QEMU backtraces. Without their guidence, tracking down
> this bug would have taken much longer.
> 
> Signed-off-by: Richard Yao <ryao@gentoo.org>
> Acked-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Will Deacon <will.deacon@arm.com>

Applied, thanks.

Re: [PATCH] 9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 30 Jan 2014 16:29:26 -0800 (PST)

> From: Richard Yao <ryao@gentoo.org>
> Date: Thu, 30 Jan 2014 13:02:48 -0500
> 
>> The 9p-virtio transport does zero copy on things larger than 1024 bytes
>> in size. It accomplishes this by returning the physical addresses of
>> pages to the virtio-pci device. At present, the translation is usually a
>> bit shift.
>> 
>> However, that approach produces an invalid page address when we
>> read/write to vmalloc buffers, such as those used for Linux kernle
>> modules. This causes QEMU to die printing:
>> 
>> qemu-system-x86_64: virtio: trying to map MMIO memory
>> 
>> This patch enables 9p-virtio to correctly handle this case. This not
>> only enables us to load Linux kernel modules off virtfs, but also
>> enables ZFS file-based vdevs on virtfs to be used without killing QEMU.
>> 
>> Also, special thanks to both Avi Kivity and Alexander Graf for their
>> interpretation of QEMU backtraces. Without their guidence, tracking down
>> this bug would have taken much longer.
>> 
>> Signed-off-by: Richard Yao <ryao@gentoo.org>
>> Acked-by: Alexander Graf <agraf@suse.de>
>> Reviewed-by: Will Deacon <will.deacon@arm.com>
> 
> Applied, thanks.

Actually I had to revert, is_vmalloc_or_malloc_addr() is not exported to
modules, so this change breaks the build.

Re: [PATCH] 9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers

[Richard Yao]

On Jan 30, 2014, at 7:44 PM, David Miller <davem@davemloft.net> wrote:

> From: David Miller <davem@davemloft.net>
> Date: Thu, 30 Jan 2014 16:29:26 -0800 (PST)
> 
>> From: Richard Yao <ryao@gentoo.org>
>> Date: Thu, 30 Jan 2014 13:02:48 -0500
>> 
>>> The 9p-virtio transport does zero copy on things larger than 1024 bytes
>>> in size. It accomplishes this by returning the physical addresses of
>>> pages to the virtio-pci device. At present, the translation is usually a
>>> bit shift.
>>> 
>>> However, that approach produces an invalid page address when we
>>> read/write to vmalloc buffers, such as those used for Linux kernle
>>> modules. This causes QEMU to die printing:
>>> 
>>> qemu-system-x86_64: virtio: trying to map MMIO memory
>>> 
>>> This patch enables 9p-virtio to correctly handle this case. This not
>>> only enables us to load Linux kernel modules off virtfs, but also
>>> enables ZFS file-based vdevs on virtfs to be used without killing QEMU.
>>> 
>>> Also, special thanks to both Avi Kivity and Alexander Graf for their
>>> interpretation of QEMU backtraces. Without their guidence, tracking down
>>> this bug would have taken much longer.
>>> 
>>> Signed-off-by: Richard Yao <ryao@gentoo.org>
>>> Acked-by: Alexander Graf <agraf@suse.de>
>>> Reviewed-by: Will Deacon <will.deacon@arm.com>
>> 
>> Applied, thanks.
> 
> Actually I had to revert, is_vmalloc_or_malloc_addr() is not exported to
> modules, so this change breaks the build.

Thanks for catching that. I had originally used is_vmalloc_addr() instead of is_vmalloc_or_malloc_addr(), but changed it after realizing this did not correct the problem on all architectures. The is_vmalloc_addr() lives in headers. I will send out a patch to get that symbol exported and resubmit this after it is merged.

[PATCH] Fix broken zero-copy on vmalloc() buffers (4th and hopefully final submission)

[Richard Yao]

This patch has been submitted for a few times.

The first time was my first time doing any sort of Linux patch
submission. At the time, I was unaware of ./scripts/get_maintainer.pl
and sent the patch to only a subset of the correct people. Consequently,
it was not submitted properly for acceptance by the subsystem maintainer.

The second time was a week ago. I had taken advice from Greg Koah-Hartman to
use ./scripts/get_maintainer.pl to determine the correct recipients. It was
initially accepted by the subsystem maintainer and then rejected. This patch
uses is_vmalloc_or_module_addr(), which is not exported for use in kernel
modules. Using it causes a build failure when CONFIG_NET_9P_VIRTIO=m is set in
.config.

The third time was earlier today, when I sent it straight to Linus Torvalds
because merging it required exporting is_vmalloc_or_module_addr(), which he
wrote. A brief correspondence with Linus revealed that my earlier belief that
it would be better to use is_vmalloc_or_module_addr() instead of
is_vmalloc_addr() was incorrect.

I expect this submission to be the last. I have changed the patch to use
is_vmalloc_addr() as Linus Torvalds suggested. This resolves the build
regression the problem David S. Miller found when CONFIG_NET_9P_VIRTIO=m was
set and should resolve all criticism.

Richard Yao (1):
  9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers

 net/9p/trans_virtio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
1.8.3.2

[PATCH] 9p/trans_virtio.c: Fix broken zero-copy on vmalloc() buffers

[Richard Yao]

The 9p-virtio transport does zero copy on things larger than 1024 bytes
in size. It accomplishes this by returning the physical addresses of
pages to the virtio-pci device. At present, the translation is usually a
bit shift.

That approach produces an invalid page address when we read/write to
vmalloc buffers, such as those used for Linux kernel modules. Any
attempt to load a Linux kernel module from 9p-virtio produces the
following stack.

[<ffffffff814878ce>] p9_virtio_zc_request+0x45e/0x510
[<ffffffff814814ed>] p9_client_zc_rpc.constprop.16+0xfd/0x4f0
[<ffffffff814839dd>] p9_client_read+0x15d/0x240
[<ffffffff811c8440>] v9fs_fid_readn+0x50/0xa0
[<ffffffff811c84a0>] v9fs_file_readn+0x10/0x20
[<ffffffff811c84e7>] v9fs_file_read+0x37/0x70
[<ffffffff8114e3fb>] vfs_read+0x9b/0x160
[<ffffffff81153571>] kernel_read+0x41/0x60
[<ffffffff810c83ab>] copy_module_from_fd.isra.34+0xfb/0x180

Subsequently, QEMU will die printing:

qemu-system-x86_64: virtio: trying to map MMIO memory

This patch enables 9p-virtio to correctly handle this case. This not
only enables us to load Linux kernel modules off virtfs, but also
enables ZFS file-based vdevs on virtfs to be used without killing QEMU.

Special thanks to both Avi Kivity and Alexander Graf for their
interpretation of QEMU backtraces. Without their guidence, tracking down
this bug would have taken much longer. Also, special thanks to Linus
Torvalds for his insightful explanation of why this should use
is_vmalloc_addr() instead of is_vmalloc_or_module_addr():

https://lkml.org/lkml/2014/2/8/272

Signed-off-by: Richard Yao <ryao@gentoo.org>
---
 net/9p/trans_virtio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index cd1e1ed..ac2666c 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -340,7 +340,10 @@ static int p9_get_mapped_pages(struct virtio_chan *chan,
 		int count = nr_pages;
 		while (nr_pages) {
 			s = rest_of_page(data);
-			pages[index++] = kmap_to_page(data);
+			if (is_vmalloc_addr(data))
+				pages[index++] = vmalloc_to_page(data);
+			else
+				pages[index++] = kmap_to_page(data);
 			data += s;
 			nr_pages--;
 		}
-- 
1.8.3.2