arm64: virt_to_page() does not return right page for a kernel image address

arm64: virt_to_page() does not return right page for a kernel image address

[Pratyush Anand]

Hi,

I noticed that on arm64 kmap_atomic() does not return correct address
corresponding to a page located in data section. It causes crash in
kdump kernel with v29 kdump patches. crash happens in a newly
implemented crypto test [1], and the same test fails(even though it
does not crash) in 1st kernel as well.

Further debugging showed that the physical address returned by
virt_to_phys(kaddr)  and virt_to_phys(kmap_atomic(virt_to_page(kaddr))
+ offset_in_page(kaddr)) are not same.

Mark Rutland thinks(IRC :#armlinux) that _virt_to_pgoff *only* handles
linear addresses, and not kernel image addresses. However, we have to
ask if it should?

Meanwhile, I reverted commit [2] and then everything worked fine
*atleast* in my case. But, I am not sure if that could be the right
and best solution.

Opinion?

~Pratyush

[1]
commit d7db7a882debaffc78f91aabedee973aa1f73390
Author: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
Date:   Fri Oct 21 13:19:54 2016 +0100

    crypto: acomp - update testmgr with support for acomp

[2]commit 9f2875912dac35d9272a82ea9eec9e5884b42cd2
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Wed Mar 30 16:46:01 2016 +0200

    arm64: mm: restrict virt_to_page() to the linear mapping

arm64: virt_to_page() does not return right page for a kernel image address

[Mark Rutland]

On Wed, Jan 04, 2017 at 11:19:38AM +0530, Pratyush Anand wrote:
> Hi,

Hi,

> I noticed that on arm64 kmap_atomic() does not return correct address
> corresponding to a page located in data section. It causes crash in
> kdump kernel with v29 kdump patches. crash happens in a newly
> implemented crypto test [1], and the same test fails(even though it
> does not crash) in 1st kernel as well.
> 
> Further debugging showed that the physical address returned by
> virt_to_phys(kaddr)  and virt_to_phys(kmap_atomic(virt_to_page(kaddr))
> + offset_in_page(kaddr)) are not same.

As I mentioned over IRC, virt_to_phys() on a kmap*() result is wrong
anyway. A kmap() results is not guaranteed to be a linear map address,
even if that's the trivial implementation used today.
 
> Mark Rutland thinks(IRC :#armlinux) that _virt_to_pgoff *only* handles
> linear addresses, and not kernel image addresses. However, we have to
> ask if it should?

>From Ard's commit, the intention is clearly that it should not,
especially given the update to virt_addr_valid(). With Larua's
DEBUG_VIRTUAL updates, we can use lm_alias() to get a linear map address
before using virt_to_page().

Other than the (new) crypto test and the (not yet upstream) kdump
patches, does any code rely on virt_to_page() working for a kernel image
address? If so, and if we cannot fix those in the short term, we may
want to temporarily revert commit 9f2875912dac35d9 until those are fixed
up.

Regardless, I think that the kdump code should not rely on
virt_to_page() for a kernel image (or kmap) result.

Thanks,
Mark.

> Meanwhile, I reverted commit [2] and then everything worked fine
> *atleast* in my case. But, I am not sure if that could be the right
> and best solution.
> 
> Opinion?
> 
> ~Pratyush
> 
> [1]
> commit d7db7a882debaffc78f91aabedee973aa1f73390
> Author: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
> Date:   Fri Oct 21 13:19:54 2016 +0100
> 
>     crypto: acomp - update testmgr with support for acomp
> 
> [2]commit 9f2875912dac35d9272a82ea9eec9e5884b42cd2
> Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Date:   Wed Mar 30 16:46:01 2016 +0200
> 
>     arm64: mm: restrict virt_to_page() to the linear mapping

arm64: virt_to_page() does not return right page for a kernel image address

[Pratyush Anand]

Hi Mark,

On Wednesday 04 January 2017 04:41 PM, Mark Rutland wrote:
> Other than the (new) crypto test and the (not yet upstream) kdump
> patches, does any code rely on virt_to_page() working for a kernel image
> address? If so, and if we cannot fix those in the short term, we may
> want to temporarily revert commit 9f2875912dac35d9 until those are fixed
> up.
>
> Regardless, I think that the kdump code should not rely on
> virt_to_page() for a kernel image (or kmap) result.

Its not the kdump code which is relying on virt_to_page() for a kernel 
image.

Its only crypto test which does that. In the first kernel (none kdump 
case) also crypto test gets a wrong kmap_atomic() address, however 
luckily there exists an entry for that address in page table and so a 
valid corresponding physical location. Therefore, it is just that we do 
not see the crash. I see still the crypto test failure print in the 1st 
kernel.
However, in crash kernel there was no valid physical address and so the 
kernel crashed.


~Pratyush

arm64: virt_to_page() does not return right page for a kernel image address

[Mark Rutland]

On Wed, Jan 04, 2017 at 05:28:32PM +0530, Pratyush Anand wrote:
> Hi Mark,

Hi Pratyush,

> On Wednesday 04 January 2017 04:41 PM, Mark Rutland wrote:
> >Other than the (new) crypto test and the (not yet upstream) kdump
> >patches, does any code rely on virt_to_page() working for a kernel image
> >address? If so, and if we cannot fix those in the short term, we may
> >want to temporarily revert commit 9f2875912dac35d9 until those are fixed
> >up.
> >
> >Regardless, I think that the kdump code should not rely on
> >virt_to_page() for a kernel image (or kmap) result.
> 
> Its not the kdump code which is relying on virt_to_page() for a
> kernel image.

Sorry, I had misread the above. I understand now.

> Its only crypto test which does that. In the first kernel (none
> kdump case) also crypto test gets a wrong kmap_atomic() address,
> however luckily there exists an entry for that address in page table
> and so a valid corresponding physical location. Therefore, it is
> just that we do not see the crash. I see still the crypto test
> failure print in the 1st kernel.
> However, in crash kernel there was no valid physical address and so
> the kernel crashed.

So it seems we need to fix the crypto test.

Looking at crypto/testmgr.c, I can't spot the kmap_atomic() or the
page_address()/virt_to_page(). I guess that's hidden behind helpers,
which might also be used elsewhere?

Could you elaborate on where exactly the problem is in the crypto test?

Thanks,
Mark.

arm64: virt_to_page() does not return right page for a kernel image address

[Catalin Marinas]

On Wed, Jan 04, 2017 at 11:19:38AM +0530, Pratyush Anand wrote:
> I noticed that on arm64 kmap_atomic() does not return correct address
> corresponding to a page located in data section. It causes crash in
> kdump kernel with v29 kdump patches. crash happens in a newly
> implemented crypto test [1], and the same test fails(even though it
> does not crash) in 1st kernel as well.
> 
> Further debugging showed that the physical address returned by
> virt_to_phys(kaddr)  and virt_to_phys(kmap_atomic(virt_to_page(kaddr))
> + offset_in_page(kaddr)) are not same.
> 
> Mark Rutland thinks(IRC :#armlinux) that _virt_to_pgoff *only* handles
> linear addresses, and not kernel image addresses. However, we have to
> ask if it should?

It looks like we have a different behaviour for virt_to_page() depending
on whether CONFIG_SPARSEMEM_VMEMMAP is defined.

We've had some discussions about a month ago on whether we should allow
virt_to_phys() on kernel addresses and requiring that __pa_symbol() is
used instead but I forgot on what the decision was (if any). It seems
that we have other cases as well that need to be addressed, in which
case it may be better to simply allow virt_to_page/phys on kernel
symbols.

-- 
Catalin

arm64: virt_to_page() does not return right page for a kernel image address

[Pratyush Anand]

On Wednesday 04 January 2017 05:36 PM, Mark Rutland wrote:
> So it seems we need to fix the crypto test.
>
> Looking at crypto/testmgr.c, I can't spot the kmap_atomic() or the
> page_address()/virt_to_page(). I guess that's hidden behind helpers,
> which might also be used elsewhere?
>
> Could you elaborate on where exactly the problem is in the crypto test?

We have in test_acomp() -> crypto_acomp_decompress() -> 
tfm->decompress() -> scomp_acomp_decompress() -> 
scomp_acomp_comp_decomp() -> scatterwalk_map_and_copy() -> 
scatterwalk_copychunks()

  41                 if (out != 2) {
  42                         vaddr = scatterwalk_map(walk);
  43                         memcpy_dir(buf, vaddr, len_this_page, out);
  44                         scatterwalk_unmap(vaddr);
  45                 }


scatterwalk_map() gets vaddr from kmap_atomic().

test_acomp() initializes sg:
sg_init_one(&src, ctemplate[i].input, ilen);

ctemplate is a kernel address (like lzo_comp_tv_template), which was 
assigned to walk->sg latter and passed to kmap_atomic().


~Pratyush

arm64: virt_to_page() does not return right page for a kernel image address

[Ard Biesheuvel]

On 4 January 2017 at 12:23, Pratyush Anand <panand@redhat.com> wrote:
>
>
> On Wednesday 04 January 2017 05:36 PM, Mark Rutland wrote:
>>
>> So it seems we need to fix the crypto test.
>>
>> Looking at crypto/testmgr.c, I can't spot the kmap_atomic() or the
>> page_address()/virt_to_page(). I guess that's hidden behind helpers,
>> which might also be used elsewhere?
>>
>> Could you elaborate on where exactly the problem is in the crypto test?
>
>
> We have in test_acomp() -> crypto_acomp_decompress() -> tfm->decompress() ->
> scomp_acomp_decompress() -> scomp_acomp_comp_decomp() ->
> scatterwalk_map_and_copy() -> scatterwalk_copychunks()
>
>  41                 if (out != 2) {
>  42                         vaddr = scatterwalk_map(walk);
>  43                         memcpy_dir(buf, vaddr, len_this_page, out);
>  44                         scatterwalk_unmap(vaddr);
>  45                 }
>
>
> scatterwalk_map() gets vaddr from kmap_atomic().
>
> test_acomp() initializes sg:
> sg_init_one(&src, ctemplate[i].input, ilen);
>

This is the bug right here, and it is fixed already upstream. The
crypto test vectors are part of the kernel image, and should not be
used in scatterlists.

> ctemplate is a kernel address (like lzo_comp_tv_template), which was
> assigned to walk->sg latter and passed to kmap_atomic().
>
>
> ~Pratyush

arm64: virt_to_page() does not return right page for a kernel image address

[Pratyush Anand]

On Wednesday 04 January 2017 06:54 PM, Ard Biesheuvel wrote:
>> sg_init_one(&src, ctemplate[i].input, ilen);
>>
> This is the bug right here, and it is fixed already upstream. The
> crypto test vectors are part of the kernel image, and should not be
> used in scatterlists.
>

Thanks a lot Ard for bringing it. I see the following commit now in 
4.10-rc2.

02608e02fbec crypto: testmgr - Use heap buffer for acomp test input

CONFIG_DEBUG_SG enabled kernel would have blown it right here.

~Pratyush

arm64: virt_to_page() does not return right page for a kernel image address

[Laura Abbott]

On 01/04/2017 04:13 AM, Catalin Marinas wrote:
> On Wed, Jan 04, 2017 at 11:19:38AM +0530, Pratyush Anand wrote:
>> I noticed that on arm64 kmap_atomic() does not return correct address
>> corresponding to a page located in data section. It causes crash in
>> kdump kernel with v29 kdump patches. crash happens in a newly
>> implemented crypto test [1], and the same test fails(even though it
>> does not crash) in 1st kernel as well.
>>
>> Further debugging showed that the physical address returned by
>> virt_to_phys(kaddr)  and virt_to_phys(kmap_atomic(virt_to_page(kaddr))
>> + offset_in_page(kaddr)) are not same.
>>

I think the underlying issue has been resolved but in general, relying
on virt_to_phys(kmap_atomic(page)) to work at all doesn't seem
correct. On arm64 and other !CONFIG_HIGHMEM systems this currently
returns the page_address but if it's actually remapped this isn't
going to work.

>> Mark Rutland thinks(IRC :#armlinux) that _virt_to_pgoff *only* handles
>> linear addresses, and not kernel image addresses. However, we have to
>> ask if it should?
> 
> It looks like we have a different behaviour for virt_to_page() depending
> on whether CONFIG_SPARSEMEM_VMEMMAP is defined.
> 

Yes, and I think the !CONFIG_SPARSEMEM_VMEMMAP is doing the correct
thing.

> We've had some discussions about a month ago on whether we should allow
> virt_to_phys() on kernel addresses and requiring that __pa_symbol() is
> used instead but I forgot on what the decision was (if any). It seems
> that we have other cases as well that need to be addressed, in which
> case it may be better to simply allow virt_to_page/phys on kernel
> symbols.
> 

So far we've been able to enforce virt_to_phys to be used only on
linear addresses and require __pa_symbol be used on kernel image symbols.
This may change once CONFIG_DEBUG_VIRTUAL gets more testing though.

This really needs to be documented somewhere though. I've added this
to my TODO list along with generally documenting some of the
virt<->phys<->page since none of that is well specified anywhere.

Thanks,
Laura

arm64: virt_to_page() does not return right page for a kernel image address

[Catalin Marinas]

On Wed, Jan 04, 2017 at 10:39:05AM -0800, Laura Abbott wrote:
> On 01/04/2017 04:13 AM, Catalin Marinas wrote:
> > On Wed, Jan 04, 2017 at 11:19:38AM +0530, Pratyush Anand wrote:
> >> I noticed that on arm64 kmap_atomic() does not return correct address
> >> corresponding to a page located in data section. It causes crash in
> >> kdump kernel with v29 kdump patches. crash happens in a newly
> >> implemented crypto test [1], and the same test fails(even though it
> >> does not crash) in 1st kernel as well.
> >>
> >> Further debugging showed that the physical address returned by
> >> virt_to_phys(kaddr)  and virt_to_phys(kmap_atomic(virt_to_page(kaddr))
> >> + offset_in_page(kaddr)) are not same.
> 
> I think the underlying issue has been resolved but in general, relying
> on virt_to_phys(kmap_atomic(page)) to work at all doesn't seem
> correct. On arm64 and other !CONFIG_HIGHMEM systems this currently
> returns the page_address but if it's actually remapped this isn't
> going to work.

Good point, kmap_atomic() does not always return an address in the
linear map, so virt_to_phys() on such address is not generally expected
to work (unrelated to arm64).

-- 
Catalin