Re: [LTP] [PATCH v4] mremap07.c: New case check mremap with MREMAP_DONTUNMAP

[Andrea Cervesato via ltp <ltp@lists.linux.it>]

Hi Wei,

The overall structure looks good, but there are a few bugs that need
fixing before this can be merged.

> +static int uffd;

This is zero-initialized by the compiler, but fd 0 is a valid file
descriptor (stdin). cleanup() guards with `if (uffd != -1)`, so if
setup() fails before SAFE_USERFAULTFD assigns a real fd, cleanup() will
call SAFE_CLOSE(0) and close stdin. Initialize it to -1:

  static int uffd = -1;

> +static void fault_handler_thread(void)

pthread start routines must have the signature `void *(*)(void *)`.
Casting a `void (*)(void)` function to that type and calling it through
the incompatible pointer is undefined behavior. Please use the correct
signature:

  static void *fault_handler_thread(void *arg LTP_ATTRIBUTE_UNUSED)
  {
          ...
          return NULL;
  }

> +	if (new_remap_addr != NULL)
> +		SAFE_MUNMAP(new_remap_addr, page_size);
> +
> +	if (fault_addr != NULL)
> +		SAFE_MUNMAP(fault_addr, page_size);

Kernel coding style: drop the `!= NULL`, write `if (new_remap_addr)`
and `if (fault_addr)`. checkpatch also flags these.

> +	SAFE_PTHREAD_CREATE(&handler_thread, NULL,
> +		(void * (*)(void *))fault_handler_thread, NULL);
> +
> +	new_remap_addr = mremap(fault_addr, page_size, page_size,
> +		MREMAP_DONTUNMAP | MREMAP_MAYMOVE);
> +
> +	if (new_remap_addr == MAP_FAILED)
> +		tst_brk(TBROK | TTERRNO, "mremap failed");

Two issues here:

1. TTERRNO prints TST_ERR, which is the errno captured by the TEST()
   macro. mremap() is called directly, so TST_ERR is stale. Use TERRNO
   (system errno) instead.

2. new_remap_addr is a static variable that is set here but only freed
   in cleanup(). When the test is run with `-i N` for N > 1, each
   iteration overwrites new_remap_addr with a fresh mremap result, and
   the previous iteration's mapping is leaked. Add an explicit munmap at
   the end of run() and reset the pointer:

     SAFE_MUNMAP(new_remap_addr, page_size);
     new_remap_addr = NULL;

> +	.needs_root = 1,

This is not needed. SAFE_USERFAULTFD() is called with retry=true, which
automatically retries with UFFD_USER_MODE_ONLY when EPERM is returned.
Everything this test does (mmap/mremap, UFFDIO_REGISTER, UFFDIO_COPY on
user pages) works without root on kernels >= 5.11. On 5.7--5.10 the
retry will result in TCONF, which is the correct behavior. Please drop
.needs_root.

Regards,
--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp