Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1524458 > unrolled thread

Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire  section Will Deacon <will.deacon@arm.com> - 2016-11-17 15:40 +0100
    Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-20 18:10 +0100
      Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-23 22:30 +0100
        Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-24 14:50 +0100
          Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-24 15:00 +0100
            Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-24 15:40 +0100
              Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-24 20:50 +0100

#1524458 — Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

On Wed, Nov 09, 2016 at 08:51:32PM +0100, Robert Richter wrote:
> Thus, I don't see where my patch breaks code. Even acpi_os_ioremap()
> keeps the same behaviour as before since it still uses memblock_is_
> memory(). Could you more describe your concerns why do you think this
> patch breaks the kernel and moves the problem somewhere else? I
> believe it fixes the problem at all.

acpi_os_ioremap always ends up in __ioremap_caller, regardless of
memblock_is_memory(). __ioremap_caller then fails if pfn_valid is true.

Will

[toc] | [next] | [standalone]

#1526257 — Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

On 17 November 2016 at 15:18, Robert Richter <robert.richter@cavium.com> wrote:
> Thanks for your answer.
>
> On 17.11.16 14:25:29, Will Deacon wrote:
>> On Wed, Nov 09, 2016 at 08:51:32PM +0100, Robert Richter wrote:
>> > Thus, I don't see where my patch breaks code. Even acpi_os_ioremap()
>> > keeps the same behaviour as before since it still uses memblock_is_
>> > memory(). Could you more describe your concerns why do you think this
>> > patch breaks the kernel and moves the problem somewhere else? I
>> > believe it fixes the problem at all.
>>
>> acpi_os_ioremap always ends up in __ioremap_caller, regardless of
>> memblock_is_memory(). __ioremap_caller then fails if pfn_valid is true.
>
> But that's the reason my patch changed the code to use memblock_is_
> map_memory() instead. I was looking into the users of pfn_valid() esp.
> in arm64 code and changed it where required.
>
> This week I looked into the kernel again for code that might break by
> a pfn_valid() change. I found try_ram_remap() in memremap.c that has
> changed behaviour now, but this is explicit for MEMREMAP_WB, so it
> should be fine.
>
> Maybe it might be better to use page_is_ram() in addition to
> pfn_valid() where necessary. This should work now after commit:
>
>  e7cd190385d1 arm64: mark reserved memblock regions explicitly in iomem
>
> I still think pfn_valid() is not the correct use to determine the mem
> attributes for mappings, there are further checks required.
>
> The risk of breaking something with my patch is small and limited only
> to the mapping of efi reserved regions (which is the state of 4.4). If
> something breaks anyway it can easily be fixed by adding more checks
> to pfn_valid() as suggested above.
>

As I noted before, it looks to me like setting CONFIG_HOLES_IN_ZONE is
the correct way to address this. However, doing that does uncover a
bug in move_freepages() where the VM_BUG_ON_PAGE() dereferences struct
page fields before the pfn_valid_within() check, so it seems those
need to be switched around.

Robert, you mentioned that CONFIG_HOLES_IN_ZONE seems inappropriate
for sparsemem. Care to elaborate why?

[toc] | [prev] | [next] | [standalone]

#1528786 — Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

On 23 November 2016 at 21:15, Robert Richter <robert.richter@cavium.com> wrote:
> On 20.11.16 17:07:44, Ard Biesheuvel wrote:
>> On 17 November 2016 at 15:18, Robert Richter <robert.richter@cavium.com> wrote:
>
>> > The risk of breaking something with my patch is small and limited only
>> > to the mapping of efi reserved regions (which is the state of 4.4). If
>> > something breaks anyway it can easily be fixed by adding more checks
>> > to pfn_valid() as suggested above.
>> >
>>
>> As I noted before, it looks to me like setting CONFIG_HOLES_IN_ZONE is
>> the correct way to address this. However, doing that does uncover a
>> bug in move_freepages() where the VM_BUG_ON_PAGE() dereferences struct
>> page fields before the pfn_valid_within() check, so it seems those
>> need to be switched around.
>>
>> Robert, you mentioned that CONFIG_HOLES_IN_ZONE seems inappropriate
>> for sparsemem. Care to elaborate why?
>
> HOLES_IN_ZONE is of rare use in the kernel. I think it was introduced
> to save memory for the memmap and only some single systems enable it.
> There is no architecture that enables it entirely. For good reasons...
>
> It introduces additional checks. pfn_valid() is usually checked only
> once for the whole memmap. There are a number of checks enabled, just
> grep for pfn_valid_within. This will increase the number of
> pfn_valid() calls by a factor of MAX_ORDER_NR_PAGES, in my config this
> is 8k. So, this is not the direction to go.
>

That does sound like a potential issue. But does it cause any slowdown
in practice?

The reality is that CONFIG_HOLES_IN_ZONE perfectly describes the
situation, and so it is still my preferred option if the performance
hit is tolerable.

> My patch fixes a regression in the kernel that was introduced by the
> nomap implementation. Some systems can not boot anymore, beside of
> that the BUG_ON() may occur any time depending only on physical page
> access, we need to fix 4.9. Here is a reproducer:
>
>  https://patchwork.kernel.org/patch/9407677/
>
> My patch also does not break memremap(). With my patch applied
> try_ram_remap() would return a linear addr for nomap regions. But this
> is only called if WB is explicitly requested, so it should not happen.

Why? MEMREMAP_WB is used often, among other things for mapping
firmware tables, which are marked as NOMAP, so in these cases, the
linear address is not mapped.

> If you think pfn_valid() is wrong here, I am happy to send a patch
> that fixes this by using page_is_ram(). In any case, the worst case
> that may happen is to behave the same as v4.4, we might fix then the
> wrong use of pfn_valid() where it is not correctly used to check for
> ram.
>

page_is_ram() uses string comparisons to look for regions called
'System RAM'. Is that something we can tolerate for each pfn_valid()
calll?

Perhaps the solution is to reimplement page_is_ram() for arm64 using
memblock_is_memory() instead, But that still means we need to modify
the generic memremap() code first to switch to it before changing the
arm64 implementation of pfn_valid

[toc] | [prev] | [next] | [standalone]

#1529296 — Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

On 24 November 2016 at 13:42, Robert Richter <robert.richter@cavium.com> wrote:
> On 23.11.16 21:25:06, Ard Biesheuvel wrote:
>> Why? MEMREMAP_WB is used often, among other things for mapping
>> firmware tables, which are marked as NOMAP, so in these cases, the
>> linear address is not mapped.
>
> If fw tables are mapped wb, that is wrong and needs a separate fix.
>

Why is that wrong?

>> > If you think pfn_valid() is wrong here, I am happy to send a patch
>> > that fixes this by using page_is_ram(). In any case, the worst case
>> > that may happen is to behave the same as v4.4, we might fix then the
>> > wrong use of pfn_valid() where it is not correctly used to check for
>> > ram.
>> >
>>
>> page_is_ram() uses string comparisons to look for regions called
>> 'System RAM'. Is that something we can tolerate for each pfn_valid()
>> calll?
>>
>> Perhaps the solution is to reimplement page_is_ram() for arm64 using
>> memblock_is_memory() instead, But that still means we need to modify
>> the generic memremap() code first to switch to it before changing the
>> arm64 implementation of pfn_valid
>
> No, that's not the solution. pfn_valid() should just check if there is
> a valid struct page, as other archs do. And if there is a mis-use of
> pfn_valid() to check for ram, only that calls should be fixed to use
> page_is_ram(), however this is implemented, or something appropriate.
> But I don't see any problematic code, and if so, I will fix that.
>

memremap() uses pfn_valid() to decide whether some address is covered
by the linear mapping. If we correct pfn_valid() to adhere to your
definition, we will need to fix memremap() first in any case.

[toc] | [prev] | [next] | [standalone]

#1529303 — Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

On 24 November 2016 at 13:51, Robert Richter <robert.richter@cavium.com> wrote:
> On 24.11.16 13:44:31, Ard Biesheuvel wrote:
>> On 24 November 2016 at 13:42, Robert Richter <robert.richter@cavium.com> wrote:
>> > On 23.11.16 21:25:06, Ard Biesheuvel wrote:
>> >> Why? MEMREMAP_WB is used often, among other things for mapping
>> >> firmware tables, which are marked as NOMAP, so in these cases, the
>> >> linear address is not mapped.
>> >
>> > If fw tables are mapped wb, that is wrong and needs a separate fix.
>> >
>>
>> Why is that wrong?
>
> The whole issue with mapping acpi tables is not marking them cachable,
> what wb does.

What 'issue'?

> Otherwise we could just use linear mapping for those mem
> ranges.
>

Regions containing firmware tables are owned by the firmware, and it
is the firmware that tells us which memory attributes we are allowed
to use. If those attributes include WB, it is perfectly legal to use a
cacheable mapping. That does *not* mean they should be covered by the
linear mapping. The linear mapping is read-write-non-exec, for
instance, and we may prefer to use a read-only mapping and/or
executable mapping.

>> >> > If you think pfn_valid() is wrong here, I am happy to send a patch
>> >> > that fixes this by using page_is_ram(). In any case, the worst case
>> >> > that may happen is to behave the same as v4.4, we might fix then the
>> >> > wrong use of pfn_valid() where it is not correctly used to check for
>> >> > ram.
>> >> >
>> >>
>> >> page_is_ram() uses string comparisons to look for regions called
>> >> 'System RAM'. Is that something we can tolerate for each pfn_valid()
>> >> calll?
>> >>
>> >> Perhaps the solution is to reimplement page_is_ram() for arm64 using
>> >> memblock_is_memory() instead, But that still means we need to modify
>> >> the generic memremap() code first to switch to it before changing the
>> >> arm64 implementation of pfn_valid
>> >
>> > No, that's not the solution. pfn_valid() should just check if there is
>> > a valid struct page, as other archs do. And if there is a mis-use of
>> > pfn_valid() to check for ram, only that calls should be fixed to use
>> > page_is_ram(), however this is implemented, or something appropriate.
>> > But I don't see any problematic code, and if so, I will fix that.
>> >
>>
>> memremap() uses pfn_valid() to decide whether some address is covered
>> by the linear mapping. If we correct pfn_valid() to adhere to your
>> definition, we will need to fix memremap() first in any case.
>
> As said, will fix that if needed. But I think the caller is wrong
> then.
>

[toc] | [prev] | [next] | [standalone]

#1529344 — Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

On 24 November 2016 at 14:11, Robert Richter <robert.richter@cavium.com> wrote:
> On 24.11.16 13:58:30, Ard Biesheuvel wrote:
>> On 24 November 2016 at 13:51, Robert Richter <robert.richter@cavium.com> wrote:
>> > On 24.11.16 13:44:31, Ard Biesheuvel wrote:
>> >> On 24 November 2016 at 13:42, Robert Richter <robert.richter@cavium.com> wrote:
>> >> > On 23.11.16 21:25:06, Ard Biesheuvel wrote:
>> >> >> Why? MEMREMAP_WB is used often, among other things for mapping
>> >> >> firmware tables, which are marked as NOMAP, so in these cases, the
>> >> >> linear address is not mapped.
>> >> >
>> >> > If fw tables are mapped wb, that is wrong and needs a separate fix.
>> >> >
>> >>
>> >> Why is that wrong?
>> >
>> > The whole issue with mapping acpi tables is not marking them cachable,
>> > what wb does.
>>
>> What 'issue'?
>>
>> > Otherwise we could just use linear mapping for those mem
>> > ranges.
>> >
>>
>> Regions containing firmware tables are owned by the firmware, and it
>> is the firmware that tells us which memory attributes we are allowed
>> to use. If those attributes include WB, it is perfectly legal to use a
>> cacheable mapping. That does *not* mean they should be covered by the
>> linear mapping. The linear mapping is read-write-non-exec, for
>> instance, and we may prefer to use a read-only mapping and/or
>> executable mapping.
>
> Ok, I am going to fix try_ram_remap().
>

Thanks. Could you also add an arm64 version of page_is_ram() that uses
memblock_is_memory() while you're at it? I think using memblock
directly in try_ram_remap() may not be the best approach

> Are there other concerns with this patch?
>

I think we all agree that pfn_valid() should return whether a pfn has
a struct page associated with it, the debate is about whether it makes
sense to allocate struct pages for memory that the kernel does not
own. But given that it does not really hurt to do so for small holes,
I think your suggestion makes sense.

Should we be doing anything more to ensure that those pages are not
dereferenced inadvertently? Is there a page flag we should be setting?

[toc] | [prev] | [next] | [standalone]

#1529656 — Re: [PATCH] arm64: mm: Fix memmap to be initialized for the entire section

On 24 November 2016 at 19:26, Robert Richter <robert.richter@cavium.com> wrote:
> Ard,
>
>> > >> On 24 November 2016 at 13:51, Robert Richter <robert.richter@cavium.com> wrote:
>> > >> > On 24.11.16 13:44:31, Ard Biesheuvel wrote:
>
>> > >> Regions containing firmware tables are owned by the firmware, and it
>> > >> is the firmware that tells us which memory attributes we are allowed
>> > >> to use. If those attributes include WB, it is perfectly legal to use a
>> > >> cacheable mapping. That does *not* mean they should be covered by the
>> > >> linear mapping. The linear mapping is read-write-non-exec, for
>> > >> instance, and we may prefer to use a read-only mapping and/or
>> > >> executable mapping.
>> > >
>> > > Ok, I am going to fix try_ram_remap().
>
> I revisited the code and it is working well already since:
>
>  e7cd190385d1 arm64: mark reserved memblock regions explicitly in iomem
>
> Now, try_ram_remap() is only called if the region to be mapped is
> entirely in IORESOURCE_SYSTEM_RAM. This is only true for normal mem
> ranges and not NOMAP mem. region_intersects() then returns
> REGION_INTERSECTS and calls try_ram_remap(). For the NOMAP memory case
> REGION_DISJOINT would be returned and thus arch_memremap_wb() being
> called directly. Before the e7cd190385d1 change try_ram_remap() was
> called also for nomap regions.
>
> So we can leave memremap() as it is and just apply this patch
> unmodified. What do you think?

I agree. The pfn_valid() check in try_ram_remap() is still appropriate
simply because the PageHighmem check requires a valid struct page. But
if we don't enter that code path anymore for NOMAP regions, I think
we're ok.

> Please ack.
>

I still don't fully understand how it is guaranteed that *all* memory
(i.e., all regions for which memblock_is_memory() returns true) is
covered by a struct page, but marked as reserved. Are we relying on
the fact that NOMAP memory is also memblock_reserve()'d?

> I am going to prepare the pfn_is_ram() change in addition to this
> patch, but that should not be required for this fix to work correcly.
>

I don't think you need to bother with page_is_ram() then. The only
place we use it is in devmem_is_allowed(), and there it makes sense to
allow NOMAP regions to be accessed (provided that you think /dev/mem
is a good idea in the first place).

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web