On 2/25/2021 10:22 AM, George Kennedy wrote:

On 2/25/2021 9:57 AM, Mike Rapoport wrote:

quoted

On Thu, Feb 25, 2021 at 07:38:19AM -0500, George Kennedy wrote:

quoted

On 2/25/2021 3:53 AM, Mike Rapoport wrote:

quoted

Hi George,

quoted

On 2/24/2021 5:37 AM, Mike Rapoport wrote:

quoted

On Tue, Feb 23, 2021 at 04:46:28PM -0500, George Kennedy wrote:

quoted

Mike,

Still no luck.

[   30.193723] iscsi: registered transport (iser)
[   30.195970] iBFT detected.
[   30.196571] BUG: unable to handle page fault for address: 
ffffffffff240004

Hmm, we cannot set ibft_addr to early pointer to the ACPI table.
Let's try something more disruptive and move the reservation back to
iscsi_ibft_find.c.

diff --git a/arch/x86/kernel/acpi/boot.c 

b/arch/x86/kernel/acpi/boot.c
index 7bdc0239a943..c118dd54a747 100644

--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c

@@ -1551,6 +1551,7 @@ void __init acpi_boot_table_init(void)

        if (acpi_disabled)
            return;
+#if 0
        /*
         * Initialize the ACPI boot-time table parser.
         */

@@ -1558,6 +1559,7 @@ void __init acpi_boot_table_init(void)

            disable_acpi();
            return;
        }
+#endif
        acpi_table_parse(ACPI_SIG_BOOT, acpi_parse_sbf);

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index d883176ef2ce..c615ce96c9a2 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c

@@ -570,16 +570,6 @@ void __init reserve_standard_io_resources(void)

    }
-static __init void reserve_ibft_region(void)
-{
-    unsigned long addr, size = 0;
-
-    addr = find_ibft_region(&size);
-
-    if (size)
-        memblock_reserve(addr, size);
-}
-
    static bool __init snb_gfx_workaround_needed(void)
    {
    #ifdef CONFIG_PCI

@@ -1032,6 +1022,12 @@ void __init setup_arch(char **cmdline_p)

         */
        find_smp_config();
+    /*
+     * Initialize the ACPI boot-time table parser.
+     */
+    if (acpi_table_init())
+        disable_acpi();
+
        reserve_ibft_region();
        early_alloc_pgt_buf();

diff --git a/drivers/firmware/iscsi_ibft_find.c 

b/drivers/firmware/iscsi_ibft_find.c
index 64bb94523281..01be513843d6 100644

--- a/drivers/firmware/iscsi_ibft_find.c
+++ b/drivers/firmware/iscsi_ibft_find.c

@@ -47,7 +47,25 @@ static const struct {

    #define VGA_MEM 0xA0000 /* VGA buffer */
    #define VGA_SIZE 0x20000 /* 128kB */
-static int __init find_ibft_in_mem(void)
+static void __init *acpi_find_ibft_region(void)
+{
+    int i;
+    struct acpi_table_header *table = NULL;
+    acpi_status status;
+
+    if (acpi_disabled)
+        return NULL;
+
+    for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++) {
+        status = acpi_get_table(ibft_signs[i].sign, 0, &table);
+        if (ACPI_SUCCESS(status))
+            return table;
+    }
+
+    return NULL;
+}
+
+static void __init *find_ibft_in_mem(void)
    {
        unsigned long pos;
        unsigned int len = 0;

@@ -70,35 +88,44 @@ static int __init find_ibft_in_mem(void)

                    /* if the length of the table extends past 1M,
                     * the table cannot be valid. */
                    if (pos + len <= (IBFT_END-1)) {
-                    ibft_addr = (struct acpi_table_ibft *)virt;
                        pr_info("iBFT found at 0x%lx.\n", pos);
-                    goto done;
+                    return virt;
                    }
                }
            }
        }
-done:
-    return len;
+
+    return NULL;
    }
+
+static void __init *find_ibft(void)
+{
+    /* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will
+     * only use ACPI for this */
+    if (!efi_enabled(EFI_BOOT))
+        return find_ibft_in_mem();
+    else
+        return acpi_find_ibft_region();
+}
+
    /*
     * Routine used to find the iSCSI Boot Format Table. The logical
     * kernel address is set in the ibft_addr global variable.
     */
-unsigned long __init find_ibft_region(unsigned long *sizep)
+void __init reserve_ibft_region(void)
    {
-    ibft_addr = NULL;
+    struct acpi_table_ibft *table;
+    unsigned long size;
-    /* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will
-     * only use ACPI for this */
+    table = find_ibft();
+    if (!table)
+        return;
-    if (!efi_enabled(EFI_BOOT))
-        find_ibft_in_mem();
-
-    if (ibft_addr) {
-        *sizep = PAGE_ALIGN(ibft_addr->header.length);
-        return (u64)virt_to_phys(ibft_addr);
-    }
+    size = PAGE_ALIGN(table->header.length);
+    memblock_reserve(virt_to_phys(table), size);
-    *sizep = 0;
-    return 0;
+    if (efi_enabled(EFI_BOOT))
+        acpi_put_table(&table->header);
+    else
+        ibft_addr = table;
    }

diff --git a/include/linux/iscsi_ibft.h b/include/linux/iscsi_ibft.h
index b7b45ca82bea..da813c891990 100644
--- a/include/linux/iscsi_ibft.h
+++ b/include/linux/iscsi_ibft.h

@@ -26,13 +26,9 @@ extern struct acpi_table_ibft *ibft_addr;

     * mapped address is set in the ibft_addr variable.
     */
    #ifdef CONFIG_ISCSI_IBFT_FIND
-unsigned long find_ibft_region(unsigned long *sizep);
+void reserve_ibft_region(void);
    #else
-static inline unsigned long find_ibft_region(unsigned long *sizep)
-{
-    *sizep = 0;
-    return 0;
-}
+static inline void reserve_ibft_region(void) {}
    #endif
    #endif /* ISCSI_IBFT_H */

Still no luck Mike,

We're back to the original problem where the only thing that 
worked was to
run "SetPageReserved(page)" before calling "kmap(page)". The page 
is being
"freed" before ibft_init() is called as a result of the recent 
buddy page
freeing changes.

I keep missing some little details each time :(

No worries. Thanks for all your help. Does this patch go on top of your
previous patch or is it standalone?

This is standalone.

quoted

George

quoted

Ok, let's try from the different angle.

diff --git a/drivers/acpi/acpica/tbutils.c 

b/drivers/acpi/acpica/tbutils.c
index 4b9b329a5a92..ec43e1447336 100644

--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c

@@ -7,6 +7,8 @@

    *
*****************************************************************************/
+#include <linux/memblock.h>
+
   #include <acpi/acpi.h>
   #include "accommon.h"
   #include "actables.h"

@@ -339,6 +341,21 @@ acpi_tb_parse_root_table(acpi_physical_address 

rsdp_address)
               acpi_tb_parse_fadt();
           }
+        if (ACPI_SUCCESS(status) &&
+ ACPI_COMPARE_NAMESEG(&acpi_gbl_root_table_list.
+                     tables[table_index].signature,
+                     ACPI_SIG_IBFT)) {
+            struct acpi_table_header *ibft;
+            struct acpi_table_desc *desc;
+
+            desc = &acpi_gbl_root_table_list.tables[table_index];
+            status = acpi_tb_get_table(desc, &ibft);
+            if (ACPI_SUCCESS(status)) {
+                memblock_reserve(address, ibft->length);
+                acpi_tb_put_table(desc);
+
+        }
+
   next_table:
           table_entry += table_entry_size;

Applied just your latest patch, but same failure.

I thought there was an earlier comment (which I can't find now) that 
stated that memblock_reserve() wouldn't reserve the page, which is 
what's needed here.

Mike,

Here was David's explanation of what he thinks is going on (or should be 
going on) from a few days ago:

QUOTE...
I assume that acpi_map()/acpi_unmap() map some firmware blob that is 
provided via firmware/bios/... to us.

should_use_kmap() tells us whether
a) we have a "struct page" and should kmap() that one
b) we don't have a "struct page" and should ioremap.

As it is a blob, the firmware should always reserve that memory region 
via memblock (e.g., memblock_reserve()), such that we either
1) don't create a memmap ("struct page") at all (-> case b) )
2) if we have to create e memmap, we mark the page PG_reserved and
*never* expose it to the buddy (-> case a) )


Are you telling me that in this case we might have a memmap for the HW 
blob that is *not* PG_reserved? In that case it most probably got 
exposed to the buddy where it can happily get allocated/freed.

The latent BUG would be that that blob gets exposed to the system like 
ordinary RAM, and not reserved via memblock early during boot. Assuming 
that blob has a low physical address, with my patch it will get 
allocated/used a lot earlier - which would mean we trigger this latent 
BUG now more easily.
...END_QUOTE

Your most recent patch has added the memblock_reserve(), but it's still 
missing the PG_reserved setting.

Thanks,
George

[   30.308229] iBFT detected..
[   30.308796] 
==================================================================
[   30.308890] BUG: KASAN: use-after-free in ibft_init+0x134/0xc33
[   30.308890] Read of size 4 at addr ffff8880be453004 by task 
swapper/0/1
[   30.308890]
[   30.308890] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 
5.11.0-f9593a0 #12
[   30.308890] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS 0.0.0 02/06/2015
[   30.308890] Call Trace:
[   30.308890]  dump_stack+0xdb/0x120
[   30.308890]  ? ibft_init+0x134/0xc33
[   30.308890]  print_address_description.constprop.7+0x41/0x60
[   30.308890]  ? ibft_init+0x134/0xc33
[   30.308890]  ? ibft_init+0x134/0xc33
[   30.308890]  kasan_report.cold.10+0x78/0xd1
[   30.308890]  ? ibft_init+0x134/0xc33
[   30.308890]  __asan_report_load_n_noabort+0xf/0x20
[   30.308890]  ibft_init+0x134/0xc33
[   30.308890]  ? write_comp_data+0x2f/0x90
[   30.308890]  ? ibft_check_initiator_for+0x159/0x159
[   30.308890]  ? write_comp_data+0x2f/0x90
[   30.308890]  ? ibft_check_initiator_for+0x159/0x159
[   30.308890]  do_one_initcall+0xc4/0x3e0
[   30.308890]  ? perf_trace_initcall_level+0x3e0/0x3e0
[   30.308890]  ? unpoison_range+0x14/0x40
[   30.308890]  ? ____kasan_kmalloc.constprop.5+0x8f/0xc0
[   30.308890]  ? kernel_init_freeable+0x420/0x652
[   30.308890]  ? __kasan_kmalloc+0x9/0x10
[   30.308890]  ? __sanitizer_cov_trace_pc+0x21/0x50
[   30.308890]  kernel_init_freeable+0x596/0x652
[   30.308890]  ? console_on_rootfs+0x7d/0x7d
[   30.308890]  ? __sanitizer_cov_trace_pc+0x21/0x50
[   30.308890]  ? rest_init+0xf0/0xf0
[   30.308890]  kernel_init+0x16/0x1d0
[   30.308890]  ? rest_init+0xf0/0xf0
[   30.308890]  ret_from_fork+0x22/0x30
[   30.308890]
[   30.308890] The buggy address belongs to the page:
[   30.308890] page:0000000001b7b17c refcount:0 mapcount:0 
mapping:0000000000000000 index:0x1 pfn:0xbe453
[   30.308890] flags: 0xfffffc0000000()
[   30.308890] raw: 000fffffc0000000 ffffea0002ef9788 ffffea0002f91488 
0000000000000000
[   30.308890] raw: 0000000000000001 0000000000000000 00000000ffffffff 
0000000000000000
[   30.308890] page dumped because: kasan: bad access detected
[   30.308890] page_owner tracks the page as freed
[   30.308890] page last allocated via order 0, migratetype Movable, 
gfp_mask 0x100dca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), pid 204, ts 
28121288605
[   30.308890]  prep_new_page+0xfb/0x140
[   30.308890]  get_page_from_freelist+0x3503/0x5730
[   30.308890]  __alloc_pages_nodemask+0x2d8/0x650
[   30.308890]  alloc_pages_vma+0xe2/0x560
[   30.308890]  __handle_mm_fault+0x930/0x26c0
[   30.308890]  handle_mm_fault+0x1f9/0x810
[   30.308890]  do_user_addr_fault+0x6f7/0xca0
[   30.308890]  exc_page_fault+0xaf/0x1a0
[   30.308890]  asm_exc_page_fault+0x1e/0x30
[   30.308890] page last free stack trace:
[   30.308890]  free_pcp_prepare+0x122/0x290
[   30.308890]  free_unref_page_list+0xe6/0x490
[   30.308890]  release_pages+0x2ed/0x1270
[   30.308890]  free_pages_and_swap_cache+0x245/0x2e0
[   30.308890]  tlb_flush_mmu+0x11e/0x680
[   30.308890]  tlb_finish_mmu+0xa6/0x3e0
[   30.308890]  exit_mmap+0x2b3/0x540
[   30.308890]  mmput+0x11d/0x450
[   30.308890]  do_exit+0xaa6/0x2d40
[   30.308890]  do_group_exit+0x128/0x340
[   30.308890]  __x64_sys_exit_group+0x43/0x50
[   30.308890]  do_syscall_64+0x37/0x50
[   30.308890]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   30.308890]
[   30.308890] Memory state around the buggy address:
[   30.308890]  ffff8880be452f00: ff ff ff ff ff ff ff ff ff ff ff ff 
ff ff ff ff
[   30.308890]  ffff8880be452f80: ff ff ff ff ff ff ff ff ff ff ff ff 
ff ff ff ff
[   30.308890] >ffff8880be453000: ff ff ff ff ff ff ff ff ff ff ff ff 
ff ff ff ff
[   30.308890]                    ^
[   30.308890]  ffff8880be453080: ff ff ff ff ff ff ff ff ff ff ff ff 
ff ff ff ff
[   30.308890]  ffff8880be453100: ff ff ff ff ff ff ff ff ff ff ff ff 
ff ff ff ff
[   30.308890] 
==================================================================

George

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel