Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Andy Lutomirski]

On Sat, Feb 8, 2014 at 9:15 AM, Richard Yao <ryao@gentoo.org> wrote:
> On 02/08/2014 01:51 AM, Andy Lutomirski wrote:
>> On Fri, Feb 7, 2014 at 11:55 AM, Dominique Martinet
>> <dominique.martinet@cea.fr> wrote:
>>Hi,
>>>
>>> Andy Lutomirski wrote on Fri, Feb 07, 2014:
>>>> I can't get modules to load from 9p.  The problem seems to be that a call like:
>>>>
>>>> kernel_read(f.file, 0, (char *)(info->hdr),, 115551);
>>>>
>>>> is filling the buffer with mostly zeros (or, more likely, just doing
>>>> nothing at all).  The call is in module.c, and the fs is mounted with:
>>>>
>>>> mount -t 9p -o ro,version=9p2000.L,trans=virtio,access=any hostroot /newroot/
>>>>
>>>> This is really easy to test: grab a copy of virtme
>>>> (https://git.kernel.org/cgit/utils/kernel/virtme/virtme.git/), build
>>>> an appropriate kernel, and run it with virtme-runkernel.  Then try to
>>>> insmod any module built for that kernel.  It won't work.
>>>>
>>>> Oddly, running executables from the same fs works, and *copying* a
>>>> module to tmpfs and insmoding it there also works.
>>>>
>>>> I'm kind of at a loss debugging this myself.  I'd expect that if
>>>> kernel_read were that broken on 9p, then I'd see more obvious
>>>> problems.
>>>>
>>>> This problem exists in at least 3.12 and a recent -linus tree.
>>>
>>> That's been reported a couple of times[1] since two months ago, there's a
>>> fix that might or might or might not make it in the tree (Eric?) there:
>>> http://www.spinics.net/lists/linux-virtualization/msg21716.html
>>>
>>> I'm pretty confident that will do it for you, but would be good to hear
>>> you confirm it again :)
>>
>> That fixes it for me.  I think it can't be a module address in
>> finit_module, though -- it's an intermediate vmalloc buffer.  It
>> could, however (in principle) be an address in module data, so the
>> full check is probably good.
>>
>> Can one of you send this to Linus and tag it for -stable?  I can
>> trigger this bug without getting an OOPS, which means that 9p is
>> overwriting random memory, which puts it in the category of rather bad
>> bugs.  I suspect that this is because I don't have
>> CONFIG_DEBUG_VIRTUAL set.
>>
>> (I can't immediately spot any code that would trigger this from user
>> space without being root, so it's probably not a security bug.)
>>
>> --Andy
>>
>
> I have already submitted it for inclusion a couple of 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.
>
> I will make a third attempt to mainline this over the next week. Later
> today, I will submit a patch exporting is_vmalloc_or_module_addr().
> After it has been accepted into mainline, I will resubmit this patch,
> which should then be accepted. This should bring this patch into Linus'
> tree sometime in the next few weeks.

I would consider asking some mm people (cc'd) how this is supposed to
work -- that is, what the appropriate way of mapping a kernel virtual
address to a struct page is.

I suspect that the answer might be unpleasant: what happens if the
address is neither in the linear map nor in vmalloc space?  For
example, it could be ioremapped.  (I have no idea under what useful
conditions the 9pnet code wants to zero-copy a buffer, but I suspect
that there are exactly zero performance-critical users of kernel_read
and kernel_write.  Presumably this is for skbs or something.)  I
suspect that the right fix is to just fall back to non-zero-copy if
the page is neither vmalloc'd nor linear-mapped, which should be
doable without new exports.

--Andy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Richard Yao]

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

On 02/08/2014 12:55 PM, Andy Lutomirski wrote:
> On Sat, Feb 8, 2014 at 9:15 AM, Richard Yao <ryao@gentoo.org> wrote:
>> On 02/08/2014 01:51 AM, Andy Lutomirski wrote:
>>> On Fri, Feb 7, 2014 at 11:55 AM, Dominique Martinet
>>> <dominique.martinet@cea.fr> wrote:
>>> Hi,
>>>>
>>>> Andy Lutomirski wrote on Fri, Feb 07, 2014:
>>>>> I can't get modules to load from 9p.  The problem seems to be that a call like:
>>>>>
>>>>> kernel_read(f.file, 0, (char *)(info->hdr),, 115551);
>>>>>
>>>>> is filling the buffer with mostly zeros (or, more likely, just doing
>>>>> nothing at all).  The call is in module.c, and the fs is mounted with:
>>>>>
>>>>> mount -t 9p -o ro,version=9p2000.L,trans=virtio,access=any hostroot /newroot/
>>>>>
>>>>> This is really easy to test: grab a copy of virtme
>>>>> (https://git.kernel.org/cgit/utils/kernel/virtme/virtme.git/), build
>>>>> an appropriate kernel, and run it with virtme-runkernel.  Then try to
>>>>> insmod any module built for that kernel.  It won't work.
>>>>>
>>>>> Oddly, running executables from the same fs works, and *copying* a
>>>>> module to tmpfs and insmoding it there also works.
>>>>>
>>>>> I'm kind of at a loss debugging this myself.  I'd expect that if
>>>>> kernel_read were that broken on 9p, then I'd see more obvious
>>>>> problems.
>>>>>
>>>>> This problem exists in at least 3.12 and a recent -linus tree.
>>>>
>>>> That's been reported a couple of times[1] since two months ago, there's a
>>>> fix that might or might or might not make it in the tree (Eric?) there:
>>>> http://www.spinics.net/lists/linux-virtualization/msg21716.html
>>>>
>>>> I'm pretty confident that will do it for you, but would be good to hear
>>>> you confirm it again :)
>>>
>>> That fixes it for me.  I think it can't be a module address in
>>> finit_module, though -- it's an intermediate vmalloc buffer.  It
>>> could, however (in principle) be an address in module data, so the
>>> full check is probably good.
>>>
>>> Can one of you send this to Linus and tag it for -stable?  I can
>>> trigger this bug without getting an OOPS, which means that 9p is
>>> overwriting random memory, which puts it in the category of rather bad
>>> bugs.  I suspect that this is because I don't have
>>> CONFIG_DEBUG_VIRTUAL set.
>>>
>>> (I can't immediately spot any code that would trigger this from user
>>> space without being root, so it's probably not a security bug.)
>>>
>>> --Andy
>>>
>>
>> I have already submitted it for inclusion a couple of 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.
>>
>> I will make a third attempt to mainline this over the next week. Later
>> today, I will submit a patch exporting is_vmalloc_or_module_addr().
>> After it has been accepted into mainline, I will resubmit this patch,
>> which should then be accepted. This should bring this patch into Linus'
>> tree sometime in the next few weeks.
> 
> I would consider asking some mm people (cc'd) how this is supposed to
> work -- that is, what the appropriate way of mapping a kernel virtual
> address to a struct page is.
> 
> I suspect that the answer might be unpleasant: what happens if the
> address is neither in the linear map nor in vmalloc space?  For
> example, it could be ioremapped.  (I have no idea under what useful
> conditions the 9pnet code wants to zero-copy a buffer, but I suspect
> that there are exactly zero performance-critical users of kernel_read
> and kernel_write.  Presumably this is for skbs or something.)  I
> suspect that the right fix is to just fall back to non-zero-copy if
> the page is neither vmalloc'd nor linear-mapped, which should be
> doable without new exports.
> 
> --Andy
> 

That is only possible if someone calls
p9_client_read()/p9_client_write() on an ioremapped address, which is an
entirely different problem.


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

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Andy Lutomirski]

On Sat, Feb 8, 2014 at 10:05 AM, Richard Yao <ryao@gentoo.org> wrote:
> On 02/08/2014 12:55 PM, Andy Lutomirski wrote:
>> On Sat, Feb 8, 2014 at 9:15 AM, Richard Yao <ryao@gentoo.org> wrote:
>>> On 02/08/2014 01:51 AM, Andy Lutomirski wrote:
>>>> On Fri, Feb 7, 2014 at 11:55 AM, Dominique Martinet
>>>> <dominique.martinet@cea.fr> wrote:
>>>> Hi,
>>>>>
>>>>> Andy Lutomirski wrote on Fri, Feb 07, 2014:
>>>>>> I can't get modules to load from 9p.  The problem seems to be that a call like:
>>>>>>
>>>>>> kernel_read(f.file, 0, (char *)(info->hdr),, 115551);
>>>>>>
>>>>>> is filling the buffer with mostly zeros (or, more likely, just doing
>>>>>> nothing at all).  The call is in module.c, and the fs is mounted with:
>>>>>>
>>>>>> mount -t 9p -o ro,version=9p2000.L,trans=virtio,access=any hostroot /newroot/
>>>>>>
>>>>>> This is really easy to test: grab a copy of virtme
>>>>>> (https://git.kernel.org/cgit/utils/kernel/virtme/virtme.git/), build
>>>>>> an appropriate kernel, and run it with virtme-runkernel.  Then try to
>>>>>> insmod any module built for that kernel.  It won't work.
>>>>>>
>>>>>> Oddly, running executables from the same fs works, and *copying* a
>>>>>> module to tmpfs and insmoding it there also works.
>>>>>>
>>>>>> I'm kind of at a loss debugging this myself.  I'd expect that if
>>>>>> kernel_read were that broken on 9p, then I'd see more obvious
>>>>>> problems.
>>>>>>
>>>>>> This problem exists in at least 3.12 and a recent -linus tree.
>>>>>
>>>>> That's been reported a couple of times[1] since two months ago, there's a
>>>>> fix that might or might or might not make it in the tree (Eric?) there:
>>>>> http://www.spinics.net/lists/linux-virtualization/msg21716.html
>>>>>
>>>>> I'm pretty confident that will do it for you, but would be good to hear
>>>>> you confirm it again :)
>>>>
>>>> That fixes it for me.  I think it can't be a module address in
>>>> finit_module, though -- it's an intermediate vmalloc buffer.  It
>>>> could, however (in principle) be an address in module data, so the
>>>> full check is probably good.
>>>>
>>>> Can one of you send this to Linus and tag it for -stable?  I can
>>>> trigger this bug without getting an OOPS, which means that 9p is
>>>> overwriting random memory, which puts it in the category of rather bad
>>>> bugs.  I suspect that this is because I don't have
>>>> CONFIG_DEBUG_VIRTUAL set.
>>>>
>>>> (I can't immediately spot any code that would trigger this from user
>>>> space without being root, so it's probably not a security bug.)
>>>>
>>>> --Andy
>>>>
>>>
>>> I have already submitted it for inclusion a couple of 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.
>>>
>>> I will make a third attempt to mainline this over the next week. Later
>>> today, I will submit a patch exporting is_vmalloc_or_module_addr().
>>> After it has been accepted into mainline, I will resubmit this patch,
>>> which should then be accepted. This should bring this patch into Linus'
>>> tree sometime in the next few weeks.
>>
>> I would consider asking some mm people (cc'd) how this is supposed to
>> work -- that is, what the appropriate way of mapping a kernel virtual
>> address to a struct page is.
>>
>> I suspect that the answer might be unpleasant: what happens if the
>> address is neither in the linear map nor in vmalloc space?  For
>> example, it could be ioremapped.  (I have no idea under what useful
>> conditions the 9pnet code wants to zero-copy a buffer, but I suspect
>> that there are exactly zero performance-critical users of kernel_read
>> and kernel_write.  Presumably this is for skbs or something.)  I
>> suspect that the right fix is to just fall back to non-zero-copy if
>> the page is neither vmalloc'd nor linear-mapped, which should be
>> doable without new exports.
>>
>> --Andy
>>
>
> That is only possible if someone calls
> p9_client_read()/p9_client_write() on an ioremapped address, which is an
> entirely different problem.
>

At the very least, calling vmalloc_to_page on a non-vmalloc module
address sounds wrong, so I don't think that exporting
is_vmalloc_or_module_address buys you anything.

--Andy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Richard Yao]

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

On 02/08/2014 02:13 PM, Andy Lutomirski wrote:
> On Sat, Feb 8, 2014 at 10:05 AM, Richard Yao <ryao@gentoo.org> wrote:
>> On 02/08/2014 12:55 PM, Andy Lutomirski wrote:
>>> On Sat, Feb 8, 2014 at 9:15 AM, Richard Yao <ryao@gentoo.org> wrote:
>>>> On 02/08/2014 01:51 AM, Andy Lutomirski wrote:
>>>>> On Fri, Feb 7, 2014 at 11:55 AM, Dominique Martinet
>>>>> <dominique.martinet@cea.fr> wrote:
>>>>> Hi,
>>>>>>
>>>>>> Andy Lutomirski wrote on Fri, Feb 07, 2014:
>>>>>>> I can't get modules to load from 9p.  The problem seems to be that a call like:
>>>>>>>
>>>>>>> kernel_read(f.file, 0, (char *)(info->hdr),, 115551);
>>>>>>>
>>>>>>> is filling the buffer with mostly zeros (or, more likely, just doing
>>>>>>> nothing at all).  The call is in module.c, and the fs is mounted with:
>>>>>>>
>>>>>>> mount -t 9p -o ro,version=9p2000.L,trans=virtio,access=any hostroot /newroot/
>>>>>>>
>>>>>>> This is really easy to test: grab a copy of virtme
>>>>>>> (https://git.kernel.org/cgit/utils/kernel/virtme/virtme.git/), build
>>>>>>> an appropriate kernel, and run it with virtme-runkernel.  Then try to
>>>>>>> insmod any module built for that kernel.  It won't work.
>>>>>>>
>>>>>>> Oddly, running executables from the same fs works, and *copying* a
>>>>>>> module to tmpfs and insmoding it there also works.
>>>>>>>
>>>>>>> I'm kind of at a loss debugging this myself.  I'd expect that if
>>>>>>> kernel_read were that broken on 9p, then I'd see more obvious
>>>>>>> problems.
>>>>>>>
>>>>>>> This problem exists in at least 3.12 and a recent -linus tree.
>>>>>>
>>>>>> That's been reported a couple of times[1] since two months ago, there's a
>>>>>> fix that might or might or might not make it in the tree (Eric?) there:
>>>>>> http://www.spinics.net/lists/linux-virtualization/msg21716.html
>>>>>>
>>>>>> I'm pretty confident that will do it for you, but would be good to hear
>>>>>> you confirm it again :)
>>>>>
>>>>> That fixes it for me.  I think it can't be a module address in
>>>>> finit_module, though -- it's an intermediate vmalloc buffer.  It
>>>>> could, however (in principle) be an address in module data, so the
>>>>> full check is probably good.
>>>>>
>>>>> Can one of you send this to Linus and tag it for -stable?  I can
>>>>> trigger this bug without getting an OOPS, which means that 9p is
>>>>> overwriting random memory, which puts it in the category of rather bad
>>>>> bugs.  I suspect that this is because I don't have
>>>>> CONFIG_DEBUG_VIRTUAL set.
>>>>>
>>>>> (I can't immediately spot any code that would trigger this from user
>>>>> space without being root, so it's probably not a security bug.)
>>>>>
>>>>> --Andy
>>>>>
>>>>
>>>> I have already submitted it for inclusion a couple of 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.
>>>>
>>>> I will make a third attempt to mainline this over the next week. Later
>>>> today, I will submit a patch exporting is_vmalloc_or_module_addr().
>>>> After it has been accepted into mainline, I will resubmit this patch,
>>>> which should then be accepted. This should bring this patch into Linus'
>>>> tree sometime in the next few weeks.
>>>
>>> I would consider asking some mm people (cc'd) how this is supposed to
>>> work -- that is, what the appropriate way of mapping a kernel virtual
>>> address to a struct page is.
>>>
>>> I suspect that the answer might be unpleasant: what happens if the
>>> address is neither in the linear map nor in vmalloc space?  For
>>> example, it could be ioremapped.  (I have no idea under what useful
>>> conditions the 9pnet code wants to zero-copy a buffer, but I suspect
>>> that there are exactly zero performance-critical users of kernel_read
>>> and kernel_write.  Presumably this is for skbs or something.)  I
>>> suspect that the right fix is to just fall back to non-zero-copy if
>>> the page is neither vmalloc'd nor linear-mapped, which should be
>>> doable without new exports.
>>>
>>> --Andy
>>>
>>
>> That is only possible if someone calls
>> p9_client_read()/p9_client_write() on an ioremapped address, which is an
>> entirely different problem.
>>
> 
> At the very least, calling vmalloc_to_page on a non-vmalloc module
> address sounds wrong, so I don't think that exporting
> is_vmalloc_or_module_address buys you anything.
> 
> --Andy
> 
The patch does not do what you describe, so we are okay.


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

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Andy Lutomirski]

On Sat, Feb 8, 2014 at 11:16 AM, Richard Yao <ryao@gentoo.org> wrote:
> On 02/08/2014 02:13 PM, Andy Lutomirski wrote:
>> On Sat, Feb 8, 2014 at 10:05 AM, Richard Yao <ryao@gentoo.org> wrote:
>>> On 02/08/2014 12:55 PM, Andy Lutomirski wrote:
>>>> On Sat, Feb 8, 2014 at 9:15 AM, Richard Yao <ryao@gentoo.org> wrote:
>>>>> On 02/08/2014 01:51 AM, Andy Lutomirski wrote:
>>>>>> On Fri, Feb 7, 2014 at 11:55 AM, Dominique Martinet
>>>>>> <dominique.martinet@cea.fr> wrote:
>>>>>> Hi,
>>>>>>>
>>>>>>> Andy Lutomirski wrote on Fri, Feb 07, 2014:
>>>>>>>> I can't get modules to load from 9p.  The problem seems to be that a call like:
>>>>>>>>
>>>>>>>> kernel_read(f.file, 0, (char *)(info->hdr),, 115551);
>>>>>>>>
>>>>>>>> is filling the buffer with mostly zeros (or, more likely, just doing
>>>>>>>> nothing at all).  The call is in module.c, and the fs is mounted with:
>>>>>>>>
>>>>>>>> mount -t 9p -o ro,version=9p2000.L,trans=virtio,access=any hostroot /newroot/
>>>>>>>>
>>>>>>>> This is really easy to test: grab a copy of virtme
>>>>>>>> (https://git.kernel.org/cgit/utils/kernel/virtme/virtme.git/), build
>>>>>>>> an appropriate kernel, and run it with virtme-runkernel.  Then try to
>>>>>>>> insmod any module built for that kernel.  It won't work.
>>>>>>>>
>>>>>>>> Oddly, running executables from the same fs works, and *copying* a
>>>>>>>> module to tmpfs and insmoding it there also works.
>>>>>>>>
>>>>>>>> I'm kind of at a loss debugging this myself.  I'd expect that if
>>>>>>>> kernel_read were that broken on 9p, then I'd see more obvious
>>>>>>>> problems.
>>>>>>>>
>>>>>>>> This problem exists in at least 3.12 and a recent -linus tree.
>>>>>>>
>>>>>>> That's been reported a couple of times[1] since two months ago, there's a
>>>>>>> fix that might or might or might not make it in the tree (Eric?) there:
>>>>>>> http://www.spinics.net/lists/linux-virtualization/msg21716.html
>>>>>>>
>>>>>>> I'm pretty confident that will do it for you, but would be good to hear
>>>>>>> you confirm it again :)
>>>>>>
>>>>>> That fixes it for me.  I think it can't be a module address in
>>>>>> finit_module, though -- it's an intermediate vmalloc buffer.  It
>>>>>> could, however (in principle) be an address in module data, so the
>>>>>> full check is probably good.
>>>>>>
>>>>>> Can one of you send this to Linus and tag it for -stable?  I can
>>>>>> trigger this bug without getting an OOPS, which means that 9p is
>>>>>> overwriting random memory, which puts it in the category of rather bad
>>>>>> bugs.  I suspect that this is because I don't have
>>>>>> CONFIG_DEBUG_VIRTUAL set.
>>>>>>
>>>>>> (I can't immediately spot any code that would trigger this from user
>>>>>> space without being root, so it's probably not a security bug.)
>>>>>>
>>>>>> --Andy
>>>>>>
>>>>>
>>>>> I have already submitted it for inclusion a couple of 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.
>>>>>
>>>>> I will make a third attempt to mainline this over the next week. Later
>>>>> today, I will submit a patch exporting is_vmalloc_or_module_addr().
>>>>> After it has been accepted into mainline, I will resubmit this patch,
>>>>> which should then be accepted. This should bring this patch into Linus'
>>>>> tree sometime in the next few weeks.
>>>>
>>>> I would consider asking some mm people (cc'd) how this is supposed to
>>>> work -- that is, what the appropriate way of mapping a kernel virtual
>>>> address to a struct page is.
>>>>
>>>> I suspect that the answer might be unpleasant: what happens if the
>>>> address is neither in the linear map nor in vmalloc space?  For
>>>> example, it could be ioremapped.  (I have no idea under what useful
>>>> conditions the 9pnet code wants to zero-copy a buffer, but I suspect
>>>> that there are exactly zero performance-critical users of kernel_read
>>>> and kernel_write.  Presumably this is for skbs or something.)  I
>>>> suspect that the right fix is to just fall back to non-zero-copy if
>>>> the page is neither vmalloc'd nor linear-mapped, which should be
>>>> doable without new exports.
>>>>
>>>> --Andy
>>>>
>>>
>>> That is only possible if someone calls
>>> p9_client_read()/p9_client_write() on an ioremapped address, which is an
>>> entirely different problem.
>>>
>>
>> At the very least, calling vmalloc_to_page on a non-vmalloc module
>> address sounds wrong, so I don't think that exporting
>> is_vmalloc_or_module_address buys you anything.
>>
>> --Andy
>>
> The patch does not do what you describe, so we are okay.

Are we looking at the same patch?

+ if (is_vmalloc_or_module_addr(data))
+ pages[index++] = vmalloc_to_page(data);

if (is_vmalloc_or_module_addr(data) && !is_vmalloc_addr(data)), the
vmalloc_to_page(data) sounds unhealthy.

--Andy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Richard Yao]

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

On 02/08/2014 02:20 PM, Andy Lutomirski wrote:
> On Sat, Feb 8, 2014 at 11:16 AM, Richard Yao <ryao@gentoo.org> wrote:
>> On 02/08/2014 02:13 PM, Andy Lutomirski wrote:
>>> On Sat, Feb 8, 2014 at 10:05 AM, Richard Yao <ryao@gentoo.org> wrote:
>>>> On 02/08/2014 12:55 PM, Andy Lutomirski wrote:
>>>>> On Sat, Feb 8, 2014 at 9:15 AM, Richard Yao <ryao@gentoo.org> wrote:
>>>>>> On 02/08/2014 01:51 AM, Andy Lutomirski wrote:
>>>>>>> On Fri, Feb 7, 2014 at 11:55 AM, Dominique Martinet
>>>>>>> <dominique.martinet@cea.fr> wrote:
>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Andy Lutomirski wrote on Fri, Feb 07, 2014:
>>>>>>>>> I can't get modules to load from 9p.  The problem seems to be that a call like:
>>>>>>>>>
>>>>>>>>> kernel_read(f.file, 0, (char *)(info->hdr),, 115551);
>>>>>>>>>
>>>>>>>>> is filling the buffer with mostly zeros (or, more likely, just doing
>>>>>>>>> nothing at all).  The call is in module.c, and the fs is mounted with:
>>>>>>>>>
>>>>>>>>> mount -t 9p -o ro,version=9p2000.L,trans=virtio,access=any hostroot /newroot/
>>>>>>>>>
>>>>>>>>> This is really easy to test: grab a copy of virtme
>>>>>>>>> (https://git.kernel.org/cgit/utils/kernel/virtme/virtme.git/), build
>>>>>>>>> an appropriate kernel, and run it with virtme-runkernel.  Then try to
>>>>>>>>> insmod any module built for that kernel.  It won't work.
>>>>>>>>>
>>>>>>>>> Oddly, running executables from the same fs works, and *copying* a
>>>>>>>>> module to tmpfs and insmoding it there also works.
>>>>>>>>>
>>>>>>>>> I'm kind of at a loss debugging this myself.  I'd expect that if
>>>>>>>>> kernel_read were that broken on 9p, then I'd see more obvious
>>>>>>>>> problems.
>>>>>>>>>
>>>>>>>>> This problem exists in at least 3.12 and a recent -linus tree.
>>>>>>>>
>>>>>>>> That's been reported a couple of times[1] since two months ago, there's a
>>>>>>>> fix that might or might or might not make it in the tree (Eric?) there:
>>>>>>>> http://www.spinics.net/lists/linux-virtualization/msg21716.html
>>>>>>>>
>>>>>>>> I'm pretty confident that will do it for you, but would be good to hear
>>>>>>>> you confirm it again :)
>>>>>>>
>>>>>>> That fixes it for me.  I think it can't be a module address in
>>>>>>> finit_module, though -- it's an intermediate vmalloc buffer.  It
>>>>>>> could, however (in principle) be an address in module data, so the
>>>>>>> full check is probably good.
>>>>>>>
>>>>>>> Can one of you send this to Linus and tag it for -stable?  I can
>>>>>>> trigger this bug without getting an OOPS, which means that 9p is
>>>>>>> overwriting random memory, which puts it in the category of rather bad
>>>>>>> bugs.  I suspect that this is because I don't have
>>>>>>> CONFIG_DEBUG_VIRTUAL set.
>>>>>>>
>>>>>>> (I can't immediately spot any code that would trigger this from user
>>>>>>> space without being root, so it's probably not a security bug.)
>>>>>>>
>>>>>>> --Andy
>>>>>>>
>>>>>>
>>>>>> I have already submitted it for inclusion a couple of 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.
>>>>>>
>>>>>> I will make a third attempt to mainline this over the next week. Later
>>>>>> today, I will submit a patch exporting is_vmalloc_or_module_addr().
>>>>>> After it has been accepted into mainline, I will resubmit this patch,
>>>>>> which should then be accepted. This should bring this patch into Linus'
>>>>>> tree sometime in the next few weeks.
>>>>>
>>>>> I would consider asking some mm people (cc'd) how this is supposed to
>>>>> work -- that is, what the appropriate way of mapping a kernel virtual
>>>>> address to a struct page is.
>>>>>
>>>>> I suspect that the answer might be unpleasant: what happens if the
>>>>> address is neither in the linear map nor in vmalloc space?  For
>>>>> example, it could be ioremapped.  (I have no idea under what useful
>>>>> conditions the 9pnet code wants to zero-copy a buffer, but I suspect
>>>>> that there are exactly zero performance-critical users of kernel_read
>>>>> and kernel_write.  Presumably this is for skbs or something.)  I
>>>>> suspect that the right fix is to just fall back to non-zero-copy if
>>>>> the page is neither vmalloc'd nor linear-mapped, which should be
>>>>> doable without new exports.
>>>>>
>>>>> --Andy
>>>>>
>>>>
>>>> That is only possible if someone calls
>>>> p9_client_read()/p9_client_write() on an ioremapped address, which is an
>>>> entirely different problem.
>>>>
>>>
>>> At the very least, calling vmalloc_to_page on a non-vmalloc module
>>> address sounds wrong, so I don't think that exporting
>>> is_vmalloc_or_module_address buys you anything.
>>>
>>> --Andy
>>>
>> The patch does not do what you describe, so we are okay.
> 
> Are we looking at the same patch?
> 
> + if (is_vmalloc_or_module_addr(data))
> + pages[index++] = vmalloc_to_page(data);
> 
> if (is_vmalloc_or_module_addr(data) && !is_vmalloc_addr(data)), the
> vmalloc_to_page(data) sounds unhealthy.
> 
> --Andy
> 

Mainline loads all Linux kernel modules into virtual memory. No
architecture is known to me where this is not the case.


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

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Andy Lutomirski]

On Sat, Feb 8, 2014 at 11:27 AM, Richard Yao <ryao@gentoo.org> wrote:
> On 02/08/2014 02:20 PM, Andy Lutomirski wrote:
>> Are we looking at the same patch?
>>
>> + if (is_vmalloc_or_module_addr(data))
>> + pages[index++] = vmalloc_to_page(data);
>>
>> if (is_vmalloc_or_module_addr(data) && !is_vmalloc_addr(data)), the
>> vmalloc_to_page(data) sounds unhealthy.
>>
>> --Andy
>>
>
> Mainline loads all Linux kernel modules into virtual memory. No
> architecture is known to me where this is not the case.
>

Hmm.  I stand corrected.  vmalloc_to_page is safe on module addresses.

--Andy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [V9fs-developer] finit_module broken on 9p because kernel_read doesn't work?

[Richard Yao]

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

On 02/08/2014 02:54 PM, Andy Lutomirski wrote:
> On Sat, Feb 8, 2014 at 11:27 AM, Richard Yao <ryao@gentoo.org> wrote:
>> On 02/08/2014 02:20 PM, Andy Lutomirski wrote:
>>> Are we looking at the same patch?
>>>
>>> + if (is_vmalloc_or_module_addr(data))
>>> + pages[index++] = vmalloc_to_page(data);
>>>
>>> if (is_vmalloc_or_module_addr(data) && !is_vmalloc_addr(data)), the
>>> vmalloc_to_page(data) sounds unhealthy.
>>>
>>> --Andy
>>>
>>
>> Mainline loads all Linux kernel modules into virtual memory. No
>> architecture is known to me where this is not the case.
>>
> 
> Hmm.  I stand corrected.  vmalloc_to_page is safe on module addresses.
> 
> --Andy
> 

I also stand corrected. After you poked me on this, I sent this patch
with a second patch to export is_vmalloc_or_module_addr() to Linus
Torvalds, who wrote is_vmalloc_or_module_addr(). He provided a very
concise explanation why is_vmalloc_addr() is not only safe, but preferable:

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

I have resubmitted it with that change. I expect it to be merged soon.


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