| Message ID | 20220414171529.62058-1-sidhartha.kumar@oracle.com |
|---|---|
| Headers | show |
| Series | selftest/vm fix segfault in mremap_test | expand |
On 4/14/22 11:15 AM, Sidhartha Kumar wrote: > Avoid calling mmap with requested addresses that > are less than the system's mmap_min_addr. Running > the test as root returns EACCES when trying to map > addresses < mmap_min_addr which is not one of the > error codes for the retry condition. Add a munmap > call after an alignment check as the mappings are > retained after the retry and can reach vm.max_map_count. > Please use 75 or 76 chars per line in change logs to make it easier to read the commit log. The description is a bit confusing. What happens when mmap tries to map addresses < mmap_min_addr? > Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com> > --- > tools/testing/selftests/vm/mremap_test.c | 41 +++++++++++++++++++++++- > 1 file changed, 40 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/vm/mremap_test.c b/tools/testing/selftests/vm/mremap_test.c > index 0624d1bd71b5..58600fee4b81 100644 > --- a/tools/testing/selftests/vm/mremap_test.c > +++ b/tools/testing/selftests/vm/mremap_test.c > @@ -6,6 +6,7 @@ > > #include <errno.h> > #include <stdlib.h> > +#include <stdio.h> > #include <string.h> > #include <sys/mman.h> > #include <time.h> > @@ -64,6 +65,35 @@ enum { > .expect_failure = should_fail \ > } > > +/* Returns mmap_min_addr sysctl */ Change this to "sysctl tunable from procfs" > +static unsigned long long get_mmap_min_addr(void) > +{ > + FILE *fp; > + int n_matched; > + static unsigned long long addr; > + > + if (addr) > + return addr; > + > + fp = fopen("/proc/sys/vm/mmap_min_addr", "r"); > + if (fp == NULL) { > + ksft_print_msg("Failed to open /proc/sys/vm/mmap_min_addr: %s\n", > + strerror(errno)); > + exit(KSFT_SKIP); > + } > + > + n_matched = fscanf(fp, "%llu", &addr); > + if (n_matched != 1) { > + ksft_print_msg("Failed to read /proc/sys/vm/mmap_min_addr: %s\n", > + strerror(errno)); > + fclose(fp); > + exit(KSFT_SKIP); > + } > + > + fclose(fp); > + return addr; > +} > + > /* > * Returns the start address of the mapping on success, else returns > * NULL on failure. > @@ -72,8 +102,15 @@ static void *get_source_mapping(struct config c) > { > unsigned long long addr = 0ULL; > void *src_addr = NULL; > + unsigned long long mmap_min_addr; > + > + mmap_min_addr = get_mmap_min_addr(); > + > retry: > addr += c.src_alignment; > + if (addr < mmap_min_addr) > + goto retry; Should this be an error or retry? Can you add why this is a retry when addr < mmap_min_addr? > + > src_addr = mmap((void *) addr, c.region_size, PROT_READ | PROT_WRITE, > MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED, > -1, 0); > @@ -91,8 +128,10 @@ static void *get_source_mapping(struct config c) > * alignment in the tests. > */ > if (((unsigned long long) src_addr & (c.src_alignment - 1)) || > - !((unsigned long long) src_addr & c.src_alignment)) > + !((unsigned long long) src_addr & c.src_alignment)) { > + munmap(src_addr, c.region_size); > goto retry; > + } > > if (!src_addr) > goto error; > thanks, -- Shuah
On 4/14/22 2:19 PM, Shuah Khan wrote: > On 4/14/22 11:15 AM, Sidhartha Kumar wrote: >> Avoid calling mmap with requested addresses that >> are less than the system's mmap_min_addr. Running >> the test as root returns EACCES when trying to map >> addresses < mmap_min_addr which is not one of the >> error codes for the retry condition. Add a munmap >> call after an alignment check as the mappings are >> retained after the retry and can reach vm.max_map_count. >> > > Please use 75 or 76 chars per line in change logs to make it > easier to read the commit log. Sure > The description is a bit confusing. What happens when mmap tries > to map addresses < mmap_min_addr? > If run without root, mmap returns the EPERM error code but with root it returns the EACCES error code. In the code of the test case, it checks if the map failed with the EPERM error code and will retry with a new address. The test breaks when run as root because EACCES isn't caught for the retry condition. Rather than arbitrarily also catching the EACCES error code, I avoid mapping with addresses < mmap_min_addr. >> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com> >> --- >> tools/testing/selftests/vm/mremap_test.c | 41 +++++++++++++++++++++++- >> 1 file changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/vm/mremap_test.c >> b/tools/testing/selftests/vm/mremap_test.c >> index 0624d1bd71b5..58600fee4b81 100644 >> --- a/tools/testing/selftests/vm/mremap_test.c >> +++ b/tools/testing/selftests/vm/mremap_test.c >> @@ -6,6 +6,7 @@ >> #include <errno.h> >> #include <stdlib.h> >> +#include <stdio.h> >> #include <string.h> >> #include <sys/mman.h> >> #include <time.h> >> @@ -64,6 +65,35 @@ enum { >> .expect_failure = should_fail \ >> } >> +/* Returns mmap_min_addr sysctl */ > > Change this to "sysctl tunable from procfs" > >> +static unsigned long long get_mmap_min_addr(void) >> +{ >> + FILE *fp; >> + int n_matched; >> + static unsigned long long addr; >> + >> + if (addr) >> + return addr; >> + >> + fp = fopen("/proc/sys/vm/mmap_min_addr", "r"); >> + if (fp == NULL) { >> + ksft_print_msg("Failed to open /proc/sys/vm/mmap_min_addr: >> %s\n", >> + strerror(errno)); >> + exit(KSFT_SKIP); >> + } >> + >> + n_matched = fscanf(fp, "%llu", &addr); >> + if (n_matched != 1) { >> + ksft_print_msg("Failed to read /proc/sys/vm/mmap_min_addr: >> %s\n", >> + strerror(errno)); >> + fclose(fp); >> + exit(KSFT_SKIP); >> + } >> + >> + fclose(fp); >> + return addr; >> +} >> + >> /* >> * Returns the start address of the mapping on success, else returns >> * NULL on failure. >> @@ -72,8 +102,15 @@ static void *get_source_mapping(struct config c) >> { >> unsigned long long addr = 0ULL; >> void *src_addr = NULL; >> + unsigned long long mmap_min_addr; >> + >> + mmap_min_addr = get_mmap_min_addr(); >> + >> retry: >> addr += c.src_alignment; >> + if (addr < mmap_min_addr) >> + goto retry; > > Should this be an error or retry? Can you add why this is a retry > when addr < mmap_min_addr? > In the original code, addr starts at 0, attempts an mmap, and retries on failure with addr += c.src_alignment. I just retry earlier before the mmap call because the mmap call will always fail if the addr < mmap_min_addr. >> + >> src_addr = mmap((void *) addr, c.region_size, PROT_READ | >> PROT_WRITE, >> MAP_FIXED_NOREPLACE | MAP_ANONYMOUS | MAP_SHARED, >> -1, 0); >> @@ -91,8 +128,10 @@ static void *get_source_mapping(struct config c) >> * alignment in the tests. >> */ >> if (((unsigned long long) src_addr & (c.src_alignment - 1)) || >> - !((unsigned long long) src_addr & c.src_alignment)) >> + !((unsigned long long) src_addr & c.src_alignment)) { >> + munmap(src_addr, c.region_size); >> goto retry; >> + } >> if (!src_addr) >> goto error; >> > > thanks, > -- Shuah
On 4/14/22 11:15 AM, Sidhartha Kumar wrote: > Allow the mremap test to be skipped due to errors > such as failing to find a valid remap region and > failure to parse the mmap_min_addr sysctl. > > Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com> > --- > tools/testing/selftests/vm/run_vmtests.sh | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh > index 88e15fbb5027..eae98f5de2cc 100755 > --- a/tools/testing/selftests/vm/run_vmtests.sh > +++ b/tools/testing/selftests/vm/run_vmtests.sh > @@ -272,11 +272,16 @@ echo "-------------------" > echo "running mremap_test" > echo "-------------------" > ./mremap_test > -if [ $? -ne 0 ]; then > +ret_val=$? > + > +if [ $ret_val -eq 0 ]; then > + echo "[PASS]" > +elif [ $ret_val -eq $ksft_skip ]; then > + echo "[SKIP]" > + exitcode=$ksft_skip > +else > echo "[FAIL]" > exitcode=1 > -else > - echo "[PASS]" > fi > > echo "-----------------" > Thank you. Looks good to me. Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah