Re: [PATCH v3 0/7] hugetlbfs memory HW error fixes

[William Roche <william.roche@oracle.com>]

On 12/2/24 17:00, David Hildenbrand wrote:
> On 02.12.24 16:41, William Roche wrote:
>> Hello David,
> 
> Hi,
> 
> sorry for reviewing yet, I was rather sick the last 1.5 weeks.

I hope you get well soon!

>> I've finally tested many page mapping possibilities and tried to
>> identify the error injection reaction on these pages to see if mmap()
>> can be used to recover the impacted area.
>> I'm using the latest upstream kernel I have for that:
>> 6.12.0-rc7.master.20241117.ol9.x86_64
>> But I also got similar results with a kernel not supporting
>> MADV_DONTNEED, for example: 5.15.0-301.163.5.2.el9uek.x86_64
>>
>>
>> Let's start with mapping a file without modifying the mapped area:
>> In this case we should have a clean page cache mapped in the process.
>> If an error is injected on this page, the kernel doesn't even inform the
>> process about the error as the page is replaced (no matter if the
>> mapping was shared of not).
>>
>> The kernel indicates this situation with the following messages:
>>
>> [10759.371701] Injecting memory failure at pfn 0x10d88e
>> [10759.374922] Memory failure: 0x10d88e: corrupted page was clean:
>> dropped without side effects
>> [10759.377525] Memory failure: 0x10d88e: recovery action for clean LRU
>> page: Recovered
> 
> Right. The reason here is that we can simply allocate a new page and 
> load data from disk. No corruption.
> 
>>
>>
>> Now when the page content is modified, in the case of standard page
>> size, we need to consider a MAP_PRIVATE or MAP_SHARED
>> - in the case of a MAP_PRIVATE page, this page is corrupted and the
>> modified data are lost, the kernel will use the SIGBUS mechanism to
>> inform this process if needed.
>>     But remapping the area sweeps away the poisoned page, and allows the
>> process to use the area.
>>
>> - In the case of a MAP_SHARED page, if the content hasn't been sync'ed
>> with the file backend, we also loose the modified data, and the kernel
>> can also raise SIGBUS.
>>     Remapping the area recreates a page cache from the "on disk" file
>> content, clearing the error.
> 
> In a mmap(MAP_SHARED, fd) region that will also require fallocate IIUC.

I would have expected the same thing, but what I noticed is that in the 
case of !hugetlb, even poisoned shared memory seem to be recovered with:
mmap(location, size, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_FIXED, fd, 0)

But we can decide that the normal behavior in this case would be to 
require an fallocate() call, and if this call fails, we fail the recovery.
My tests showed that a standard sized page can be replaced by a new one 
calling the above mmap(). And shared hugetlb case doesn't work this way.

>>
>> In both cases, the kernel indicates messages like:
>> [41589.578750] Injecting memory failure for pfn 0x122105 at process
>> virtual address 0x7f13bad55000
>> [41589.582237] Memory failure: 0x122105: Sending SIGBUS to testdh:7343
>> due to hardware memory corruption
>> [41589.584907] Memory failure: 0x122105: recovery action for dirty LRU
>> page: Recovered
>  > >
>> Now in the case of hugetlbfs pages:
>> This case behaves the same way as the standard page size when using
>> MAP_PRIVATE: mmap of the underlying file is able to sweep away the
>> poisoned page.
>> But the MAP_SHARED case is different: mmap() doesn't clear anything.
>> fallocate() must be used.
> 
> Yes, I recall that is what I initially said. The behavior with 
> MAP_SHARED should be consistent between hugetlb and !hugetlb.

The tests showed that they are different.

>>
>>
>> In both cases, the kernel indicates messages like:
>> [89141.724295] Injecting memory failure for pfn 0x117800 at process
>> virtual address 0x7fd148800000
>> [89141.727103] Memory failure: 0x117800: Sending SIGBUS to testdh:9480
>> due to hardware memory corruption
>> [89141.729829] Memory failure: 0x117800: recovery action for huge page:
>> Recovered
>>
>> Conclusion:
>> We can't count on the mmap() method only for the hugetlbfs case with
>> MAP_SHARED.
>>

At the end of this email, I included the source code of a simplistic 
test case that shows that the page is replaced in the case of standard 
page size.

The idea of this test is simple:

1/ Create a local FILE with:
# dd if=/dev/zero of=./FILE bs=4k count=2
2+0 records in
2+0 records out
8192 bytes (8.2 kB, 8.0 KiB) copied, 0.000337674 s, 24.3 MB/s

2/ As root run:
# ./poisonedShared4k
Mapping 8192 bytes from file FILE
Reading and writing the first 2 pages content:
Read: Read: Wrote: Initial mem page 0
Wrote: Initial mem page 1
Data pages at 0x7f71a19d6000  physically 0x124fb0000
Data pages at 0x7f71a19d7000  physically 0x128ce4000
Poisoning 4k at 0x7f71a19d6000
Signal 7 received
	code 4		Signal code
	addr 0x7f71a19d6000	Memory location
	si_addr_lsb 12
siglongjmp used
Remapping the poisoned page
Reading and writing the first 2 pages content:
Read: Read: Initial mem page 1
Wrote: Rewrite mem page 0
Wrote: Rewrite mem page 1
Data pages at 0x7f71a19d6000  physically 0x10c367000
Data pages at 0x7f71a19d7000  physically 0x128ce4000


  ---

As we can see, this process:
- maps the FILE,
- tries to read and write the beginning of the first 2 pages
- gives their physical addresses
- poison the first page with a madvise(MADV_HWPOISON) call
- shows the SIGBUS signal received and recovers from it
- simply remaps the same page from the file
- tries again to read and write the beginning of the first 2 pages
- gives their physical addresses

  ---

The test (run on 6.12.0-rc7.master.20241117.ol9.x86_64) showed the 
memory is usable after the remap.
Do you see a different behavior, with an even more recent kernel ?

 >> So According to these tests results, we should change the part of the
 >> qemu_ram_remap() function (in the 2nd patch) to something like:
 >>
 >> +                if (ram_block_discard_range(block, offset + 
block->fd_offset,
 >> +                                            length) != 0) {
 >> +                    /*
 >> +                     * Fold back to using mmap(), but it cannot 
repair a
 >> +                     * shared hugetlbfs region. In this case we fail.
 >> +                     */
 >
 >
 > But why do we special-case hugetlb here? How would mmap(MAP_FIXED) help
 > to discard dirty pagecache data in a mmap(MAD_SHARED, fd) mapping?

You can see the behavior with the test case.

But for Qemu, we could decide to ignore that, and choose to fail in the 
generic case:

+                    /*
+                     * Fold back to using mmap(), but it should not 
repair a
+                     * shared file memory region. In this case we fail.
+                     */
+                    if (block->fd >= 0 && qemu_ram_is_shared(block)) {
+                        error_report("Shared memory poison recovery 
failure addr: "
+                                     RAM_ADDR_FMT "@" RAM_ADDR_FMT "",
+                                     length, addr);
+                        exit(1);
+                    }

Do you think this would be more secure ?

HTH,
William.


  ---------------------------------

#include <sys/types.h>
#include <sys/mman.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <stdint.h>
#include <signal.h>
#include <string.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <setjmp.h>

#define PAGEMAP_ENTRY 8
#define GET_BIT(X,Y) (X & ((uint64_t)1<<Y)) >> Y
#define GET_PFN(X) X & 0x7FFFFFFFFFFFFF
const int __endian_bit = 1;
#define is_bigendian() ( (*(char*)&__endian_bit) == 0 )

#define ALLOC_PAGES 2
#define myFile "FILE"
static sigjmp_buf jmpbuf;

/*
  * Generate an error on the given page.
  */
static void memory_error_advise(void* virtual_page) {
    int ret;

    printf("Poisoning 4k at %p\n", virtual_page);
    if (sigsetjmp(jmpbuf, 1) == 0) {
       ret = madvise(virtual_page, 4096, MADV_HWPOISON);
       if (ret)
          printf("Poisoning failed - madvise: %s", strerror(errno));
    }
}

static void print_physical_address(uint64_t virt_addr) {
    char path_buf [0x100];
    FILE * f;
    uint64_t read_val, file_offset, pfn = 0;
    long pgsz;
    unsigned char c_buf[PAGEMAP_ENTRY];
    pid_t my_pid = getpid();
    int status, i;

    sprintf(path_buf, "/proc/%u/pagemap", my_pid);

    f = fopen(path_buf, "rb");
    if(!f){
       printf("Error! Cannot open %s\n", path_buf);
       exit(EXIT_FAILURE);
    }

    pgsz = getpagesize();
    file_offset = virt_addr / pgsz * PAGEMAP_ENTRY;
    status = fseek(f, file_offset, SEEK_SET);
    if(status){
       perror("Failed to do fseek!");
       fclose(f);
       exit(EXIT_FAILURE);
    }

    for(i=0; i < PAGEMAP_ENTRY; i++){
       int c = getc(f);
       if(c==EOF){
          fclose(f);
          exit(EXIT_FAILURE);
       }
       if(is_bigendian())
            c_buf[i] = c;
       else
            c_buf[PAGEMAP_ENTRY - i - 1] = c;
    }
    fclose(f);

    read_val = 0;
    for(i=0; i < PAGEMAP_ENTRY; i++){
       read_val = (read_val << 8) + c_buf[i];
    }

    if(GET_BIT(read_val, 63)) { // Bit 63 page present
       pfn = GET_PFN(read_val);
    } else {
       printf("Page not present !\n");
    }
    if(GET_BIT(read_val, 62)) // Bit 62 page swapped
       printf("Page swapped\n");

    if (pfn == 0) {
       printf("Virt address translation 0x%llx failed\n");
       exit(EXIT_FAILURE);
    }

    printf("Data pages at 0x%llx  physically 0x%llx\n",
          (unsigned long long)virt_addr, (unsigned long long)pfn * pgsz);
}

/*
  * SIGBUS handler to display the given information.
  */
static void sigbus_action(int signum, siginfo_t *siginfo, void *ctx) {
    printf("Signal %d received\n", signum);
    printf("\tcode %d\t\tSignal code\n", siginfo->si_code);
    printf("\taddr 0x%llx\tMemory location\n", siginfo->si_addr);
    printf("\tsi_addr_lsb %d\n", siginfo->si_addr_lsb);

   if (siginfo->si_code == 4) { /* BUS_MCEERR_AR */
	fprintf(stderr, "siglongjmp used\n");
	siglongjmp(jmpbuf, 1);
   }
}

static void read_write(void* addr, int nb_pages, char* prefix) {
    int i;
    fprintf(stderr, "Reading and writing the first %d pages content:\n", 
nb_pages);
    if (sigsetjmp(jmpbuf, 1) == 0) {
       // read the strings at the beginning of each page.
       for (i=0; i < nb_pages; i++) {
          printf("Read: %s", ((char *)addr+ i*4096));
       }
       // also write something
       for (i=0; i < 2; i++) {
          sprintf(((char *)addr + i*4096), "%s %d\n", prefix, i);
	 printf("Wrote: %s %d\n", prefix, i);
       }
    }
}

int main(int argc, char ** argv) {
    int opt, fd, i;
    struct sigaction my_sigaction;
    uint64_t virt_addr, phys_addr;
    void *local_pnt, *v;
    struct stat statbuf;
    off_t s;

    // Need to have the CAP_SYS_ADMIN capability to get PFNs values in 
pagemap.
    if (getuid() != 0) {
       fprintf(stderr, "Usage: %s needs to run as root\n", argv[0]);
       exit(EXIT_FAILURE);
    }

    // attach our SIGBUS handler.
    my_sigaction.sa_sigaction = sigbus_action;
    my_sigaction.sa_flags = SA_SIGINFO | SA_NODEFER | SA_SIGINFO;
    if (sigaction(SIGBUS, &my_sigaction, NULL) == -1) {
       perror("Signal handler attach failed");
       exit(EXIT_FAILURE);
    }

    fd = open(myFile, O_RDWR);
    if (fd == -1) {
       perror("open");
       exit(EXIT_FAILURE);
    }
    if (fstat(fd, &statbuf) == -1) {
       perror("fstat");
       exit(EXIT_FAILURE);
    }
    s = statbuf.st_size;
    if (s < 2*4096) {
      fprintf(stderr, "File must be at least 2 pages large\n");
      exit(EXIT_FAILURE);
    }

    printf("Mapping %d bytes from file %s\n", s, myFile);
    local_pnt = mmap(NULL, s, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
    if (local_pnt == MAP_FAILED) {
      perror("mmap");
      exit(EXIT_FAILURE);
    }
    read_write(local_pnt, 2, "Initial mem page");

    virt_addr = (uint64_t)local_pnt;
    print_physical_address(virt_addr);
    print_physical_address(virt_addr+getpagesize());

    // Explicit error
    memory_error_advise((void*)virt_addr);

    // Remap the poisoned page
    fprintf(stderr, "Remapping the poisoned page\n");
    v = mmap(local_pnt, 4092, PROT_READ|PROT_WRITE, 
MAP_SHARED|MAP_FIXED, fd, 0);
    if ((v == MAP_FAILED) || (v != local_pnt)) {
       perror("mmap");
    }

    read_write(local_pnt, 2, "Rewrite mem page");
    print_physical_address(virt_addr);
    print_physical_address(virt_addr+getpagesize());
    return 0;
}