Latest version of zero-copy fix

Latest version of zero-copy fix

[David Miller]

So now the patch only tests is_vmalloc_addr(), did you test this
version with the situation that triggers the given backtrace?

[<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

This is reading from v9fs into a module address.

It's going generate the same backtrace with your fix.

I don't think the situation was sufficiently explained to Linus.  In
fact, I didn't see the above backtrace mentioned at all.

Re: Latest version of zero-copy fix

[Richard Yao]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]

On 02/10/2014 06:21 PM, David Miller wrote:
> 
> So now the patch only tests is_vmalloc_addr(), did you test this
> version with the situation that triggers the given backtrace?
> 
> [<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
> 
> This is reading from v9fs into a module address.
> 
> It's going generate the same backtrace with your fix.
> 
> I don't think the situation was sufficiently explained to Linus.  In
> fact, I didn't see the above backtrace mentioned at all.
> 

I have tested it against Linux 3.13.0 and I can confirm that it solves
the problem. In fact, this was the original patch in December before I
made the last minute change to is_module_or_vmalloc_addr() only after
deciding that is_vmalloc_addr() was not general enough.

Linus' response had two key points. Linus' first point was that my
generalization of is_vmalloc_addr() to is_module_or_vmalloc_addr() was
unnecessary to fix this problem because the order of events is as follows:

1. We allocate a temporary buffer with vmalloc().
2. We read the module into that buffer.
3. We copy the module from that buffer into its final location.

Linus' second point was that my generalization permitted IO to be done
to a region of memory where IO operations should never be done. When I
wrote this back in December, I wanted to write as general a fix as
possible because Alexander Graf had pointed out to me that there had
Will Deacon had written a patch that should have included this, but
neglected it and I was afraid that that would become a cycle.

Linus' clear explanation of why this is a bad idea changed my mind, so I
submitted the original form of this patch with an updated commit message.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: Latest version of zero-copy fix

[David Miller]

From: Richard Yao <ryao@gentoo.org>
Date: Mon, 10 Feb 2014 20:31:11 -0500

> 1. We allocate a temporary buffer with vmalloc().
> 2. We read the module into that buffer.

Ok, I didn't catch this, I'll apply your patch thanks!