Helge Deller <[email protected]> writes:
> Hi Alex,
>
> On 9/3/26 16:27, Alex Bennée wrote:
>> Per madvise(2) and the Linux kernel implementation (madvise_walk_vmas),
>> madvise() must validate that the requested range is currently mapped
>> and return -ENOMEM if any page in the range is unmapped.
>> Add a page_check_range(start, len, PAGE_VALID) check for valid
>> advice
>> values before proceeding with the advice actions. In addition, extend the
>> tcg multiarch test linux-madvise.c to test this behaviour.
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4382
>> AI-used-for: importing and validating test case
>> Signed-off-by: Alex Bennée <[email protected]>
>> ---
>> NOTE
>> - again testing the minimal agents which did stop and say:
>> *(Note: Per user policy, git commits are never executed
>> automatically by the agent. Please review the diff with `git diff`
>> and commit the changes if you are satisfied.)*
>> but non-the-less imported the test and wrote a crap patch which I
>> have re-done dropping a load of unneeded verbosity.
>
>
> Did you test this patch?
> If yes, did it work for you?
Yes - ran the test case before and after the changes and it asserts
before.
./qemu-aarch64 -strace -d guest_errors,unimp
aarch64-linux-user-linux-madvise.test
681364 uname(0x7f403e54d3c8) = 0
681364 brk(NULL) = 0x00000000004a7000
681364 brk(0x00000000004a7b00) = 0x00000000004a7b00
681364 set_tid_address(0x4a70f0) = 681364
681364 set_robust_list(0x4a7100,24) = -1 errno=38 (Function not implemented)
681364 rseq(0x4a77c0,32,0,0xd428bc00)Unsupported syscall: 293
= -1 errno=38 (Function not implemented)
681364 prlimit64(0,RLIMIT_STACK,NULL,0x00007f403e54d518) = 0
({rlim_cur=8388608,rlim_max=-1})
681364 readlinkat(AT_FDCWD,"/proc/self/exe",0x00007f403e54c4a0,4096) = 73
681364 getrandom(0x4a6808,8,1) = 8
681364 brk(NULL) = 0x00000000004a7b00
681364 brk(0x00000000004c8b00) = 0x00000000004c8b00
681364 brk(0x00000000004c9000) = 0x00000000004c9000
681364 mprotect(0x000000000049b000,20480,PROT_READ) = 0
681364 mmap(NULL,4096,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) =
0x00007f40405f2000
681364 mprotect(0x00007f40405f2000,4096,PROT_READ|PROT_WRITE) = 0
681364 madvise(0x00007f40405f2000,4096,MADV_DONTNEED) = 0
681364 munmap(0x00007f40405f2000,4096) = 0
681364 getrandom(0x7f403e54d360,8,1) = 8
681364 openat(AT_FDCWD,"/tmp/.cmadvisebhAABn",O_RDWR|O_CREAT|O_EXCL,0600) = 3
681364 unlinkat(AT_FDCWD,"/tmp/.cmadvisebhAABn",0) = 0
681364 write(3,0x7f403e54d3f7,1) = 1
681364 ftruncate(3,4096) = 0
681364 mmap(NULL,4096,PROT_READ,MAP_PRIVATE,3,0) = 0x00007f40405f2000
681364 mprotect(0x00007f40405f2000,4096,PROT_READ|PROT_WRITE) = 0
681364 madvise(0x00007f40405f2000,4096,MADV_DONTNEED) = 0
681364 munmap(0x00007f40405f2000,4096) = 0
681364 close(3) = 0
681364 mmap(NULL,4096,PROT_READ,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) =
0x00007f40405f2000
681364 munmap(0x00007f40405f2000,4096) = 0
681364 madvise(0x00007f40405f2000,4096,MADV_NORMAL) = 0
681364 write(2,0x7f403e54cbc8,128)aarch64-linux-user-linux-madvise.test:
/home/alex/lsrc/qemu.git/tests/tcg/multiarch/linux/linux-madvise.c:81:
test_unmapped: Ass = 128
681364 write(2,0x7f403e54cbc8,27)ertion `ret == -1' failed.
= 27
681364 mmap(NULL,4096,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANONYMOUS,-1,0) =
0x00007f40405f2000
681364 gettid() = 681364
681364 getpid() = 681364
681364 tgkill(681364,681364,SIGIOT) = 0
--- SIGIOT {si_signo=SIGIOT, si_code=SI_TKILL, si_pid=681364, si_uid=1000} ---
qemu: uncaught target signal 6 (Aborted) - core dumped
fish: Job 1, './qemu-aarch64 -strace -d guest…' terminated by signal SIGABRT
(Abort)
> I'm asking, because I tried the testcase from the bug report, and
> in qemu I still get 0 (success).
I suspect you've been tripped up by the conversion of tests to meson as
they now have the .test suffix. So with the patch applied and tests
passing:
cp tests/tcg/aarch64-linux-user-linux-madvise.test .
and then drop back one patch and rebuild and test against the copy of
the test with the test_unmapped support.
>
> Helge
>
>> ---
>> linux-user/mmap.c | 12 ++++++++++++
>> tests/tcg/multiarch/linux/linux-madvise.c | 20 ++++++++++++++++++++
>> 2 files changed, 32 insertions(+)
>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index cc0c2ee6c27..4066072ff45 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -1307,6 +1307,16 @@ abi_long target_madvise(abi_ulong start, abi_ulong
>> len_in, int advice)
>> * though.
>> */
>> mmap_lock();
>> +
>> + /*
>> + * Whatever advice if the pages are not currently mapped, or are
>> + * outside the address space of the process.
>> + */
>> + if (!page_check_range(start, len, PAGE_VALID)) {
>> + ret = -TARGET_ENOMEM;
>> + goto unlock;
>> + }
>> +
>> switch (advice) {
>> case MADV_NORMAL:
>> case MADV_RANDOM:
>> @@ -1358,6 +1368,8 @@ abi_long target_madvise(abi_ulong start, abi_ulong
>> len_in, int advice)
>> ret = -EINVAL; /* not yet known advise */
>> break;
>> }
>> +
>> + unlock:
>> mmap_unlock();
>> return ret;
>> diff --git a/tests/tcg/multiarch/linux/linux-madvise.c
>> b/tests/tcg/multiarch/linux/linux-madvise.c
>> index 539fb3b7726..ebb9666c919 100644
>> --- a/tests/tcg/multiarch/linux/linux-madvise.c
>> +++ b/tests/tcg/multiarch/linux/linux-madvise.c
>> @@ -1,4 +1,5 @@
>> #include <assert.h>
>> +#include <errno.h>
>> #include <stdlib.h>
>> #include <sys/mman.h>
>> #include <unistd.h>
>> @@ -63,10 +64,29 @@ static void test_file(void)
>> assert(ret == 0);
>> }
>> +static void test_unmapped(void)
>> +{
>> + int pagesize = getpagesize();
>> + void *page;
>> + int ret;
>> +
>> + page = mmap(NULL, pagesize, PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1,
>> 0);
>> + assert(page != MAP_FAILED);
>> +
>> + ret = munmap(page, pagesize);
>> + assert(ret == 0);
>> +
>> + errno = 0;
>> + ret = madvise(page, pagesize, MADV_NORMAL);
>> + assert(ret == -1);
>> + assert(errno == ENOMEM);
>> +}
>> +
>> int main(void)
>> {
>> test_anonymous();
>> test_file();
>> + test_unmapped();
>> return EXIT_SUCCESS;
>> }
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
