RE: [PATCH v4 7/7] ACPI / x86: introduce acpi_os_readable() support

[Zheng, Lv]

Hi, Andy

> From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi-
> owner@xxxxxxxxxxxxxxx] On Behalf Of Andy Lutomirski
> Sent: Wednesday, December 23, 2015 6:49 AM
> Subject: Re: [PATCH v4 7/7] ACPI / x86: introduce acpi_os_readable() support
> 
> On Mon, Dec 21, 2015 at 5:03 PM, Chen, Yu C <yu.c.chen@xxxxxxxxx> wrote:
> > Hi Andy,
> > thanks for your review,
> >
> >> -----Original Message-----
> >> From: Andy Lutomirski [mailto:luto@xxxxxxxxxxxxxx]
> >> Sent: Friday, December 18, 2015 1:00 AM
> >> To: Zheng, Lv
> >> Cc: Chen, Yu C; Moore, Robert; Wysocki, Rafael J; Brown, Len; Andy
> >> Lutomirski; Lv Zheng; linux-kernel@xxxxxxxxxxxxxxx; Linux ACPI; H. Peter
> >> Anvin; Borislav Petkov
> >> Subject: Re: [PATCH v4 7/7] ACPI / x86: introduce acpi_os_readable()
> support
> >>
> > [cut]
> >>
> >> I think that hpa or Borislav [cc'd] could address the memory map details
> >> better than I could.  However, this functionality seems strange.
> >>
> >> Are these physical addresses or virtual addresses that are being dumped?
> > [Yu] They are  virtual addresses to be dumped.
> >> In  either case, ISTM that using something iike page_is_ram might be a lot
> >> simpler.
> > [Yu] if i understand correctly, this API is used to check if the address is a valid
> > 'kmalloc' style address, but not 'kmap' or 'vmalloc' address, and page_is_ram
> > might treat the latter as valid address?
> >
> 
> I'm a bit puzzled as to why this matters, but I have no fundamental objection to doing it that way.
[Lv Zheng] 
IMO, using page_is_ram() or something similar, the problem is what we need to solve in the current approach still need to be solved:
1. How can we convert a virtual address into a "struct page"?
    There is no kernel API to convert any virtual address into struct page.
    Even there is such a kernel API to convert kmap/vmalloc addresses, we still couldn't use it.
    Because if we want to validate kmap/vmaloc pages, we need 2 APIs rather than 1 API while ACPICA only provides 1 API for this purpose.
    The 2 APIs should be get/put style to ping the page mappings as the mappings other than the direct mappings will not be stationary in the kernel address space.
    Fortunately we needn't take care of the mappings other than the direct mappings (reasons are in the 2nd comment).
    So we still need to use the direct mapping APIs here.
2. How can we ensure the page is a direct mapping page?
    I think Yu should confirm if there is such a common kernel API.
    If there is such an API, we should use it so that we can remove the arch specific stuffs.

> What's the use case, though?
[Lv Zheng] 
Fortunately, currently ACPICA only uses this API to validate if a namespace node, an operand object or a parser object is readable.
See drivers/acpi/acpica/dbdisplay.c and drivers/acpi/acpica/dbcmds.c.

>  That is, what goes wrong if the function just always returns false?
[Lv Zheng] 
1. If it always returns false, then many ACPICA debugger internal object conversion/dump functionalities won't be functioning.
    For example, you can try to type âdump \_SB" in acpidbg shell and it will return an error:
      "Invalid named object at address xxxxxxxxxxxxxxxx"
2. While if this function always returns true (current linux-pm/linux-next merged stuffs), we can see such a result:
      Object (ffffxxxxxxxxxxxx) Pathname: \_SB
          Name : _SB_
          Type : 06 [Device]
          ...
3. But if it always returns true, then there will be another problem:
    User can type an invalid address, for example, "dump 0xFFFFFFFFFFFFFFFF".
    And ACPICA debugger will try to access this invalid virtual address and finally result in a panic.
    So we need to implement acpi_os_readable() to harden the check.

[Lv Zheng]
Let me say more about this patch.
Currently this patch looks wrong.
Though, most of the acpi_object(s) are kmalloced in the kernel heap, as far as I know, at least the namespace root is a statically allocated object in ACPICA.
Maybe "One"/"Ones"/"Zero" operands are all statically allocated objects.

So we need to modify this function to return true for the addresses that belong to .data/.bss sections for x86_64 kernels.
You can confirm this by typing "dump \" in the acpidbg shell, it now returns:
 "Invalid named object at address ffffffff8xxxxxxx".
We'll update it and send it after testing.

Thanks and best regards
-Lv