In msync03, sbrk(0) was used and offset to get a pointer to an unmapped area. This caused issues in purecap because sbrk is now disabled, and in compat with the 1.5 verrsion of musl because of a bug in the sbrk implementation.
Instead, don't rely on sbrk at all and mmap a page, unmap it and use this pointer in order to get an unmapped area of memory, generating an ENOMEM in compat and purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- testcases/kernel/syscalls/msync/msync03.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/testcases/kernel/syscalls/msync/msync03.c b/testcases/kernel/syscalls/msync/msync03.c index 8fa62e57a..6d59c9d26 100644 --- a/testcases/kernel/syscalls/msync/msync03.c +++ b/testcases/kernel/syscalls/msync/msync03.c @@ -130,8 +130,17 @@ static void setup(void) SAFE_GETRLIMIT(NULL, RLIMIT_DATA, &rl); addr3 = (char *)(uintptr_t)rl.rlim_max;
- /* memory pointed to by addr4 was not mapped */ - addr4 = sbrk(0) + (4 * page_sz); + /* memory pointed to by addr4 is not mapped */ + addr4 = mmap(NULL, page_sz, PROT_NONE, MAP_PRIVATE, 0, 0); + if (addr4 == MAP_FAILED) { + addr4 = NULL; + } else { + if (!munmap(addr4, page_sz)) + tst_resm(TFAIL, "Could not setup test, munmap failed :\n%s", + strerror(errno)); + } + + tst_resm(TINFO, "addr4 : %p", addr4); }
static void msync_verify(struct test_case_t *tc)
On 12/12/2022 14:59, Teo Couprie Diaz wrote:
In msync03, sbrk(0) was used and offset to get a pointer to an unmapped area. This caused issues in purecap because sbrk is now disabled, and in compat with the 1.5 verrsion of musl because of a bug in the sbrk implementation.
Instead, don't rely on sbrk at all and mmap a page, unmap it and use this pointer in order to get an unmapped area of memory, generating an ENOMEM in compat and purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/msync/msync03.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/testcases/kernel/syscalls/msync/msync03.c b/testcases/kernel/syscalls/msync/msync03.c index 8fa62e57a..6d59c9d26 100644 --- a/testcases/kernel/syscalls/msync/msync03.c +++ b/testcases/kernel/syscalls/msync/msync03.c @@ -130,8 +130,17 @@ static void setup(void) SAFE_GETRLIMIT(NULL, RLIMIT_DATA, &rl); addr3 = (char *)(uintptr_t)rl.rlim_max;
- /* memory pointed to by addr4 was not mapped */
- addr4 = sbrk(0) + (4 * page_sz);
- /* memory pointed to by addr4 is not mapped */
- addr4 = mmap(NULL, page_sz, PROT_NONE, MAP_PRIVATE, 0, 0);
- if (addr4 == MAP_FAILED) {
addr4 = NULL;- } else {
if (!munmap(addr4, page_sz))tst_resm(TFAIL, "Could not setup test, munmap failed :\n%s",strerror(errno));- }
- tst_resm(TINFO, "addr4 : %p", addr4);
I obviously forgot this debug message, it will be removed if a v2 is required (which will probably be the case) ! Apologies for the mistake.
} static void msync_verify(struct test_case_t *tc)
Hi Teo,
On Mon, Dec 12, 2022 at 02:59:42PM +0000, Teo Couprie Diaz wrote:
In msync03, sbrk(0) was used and offset to get a pointer to an unmapped area.
How about rewording this a bit with smth along the lines of:
In msync03 testcase, in order to get unmapped memory area, sbrk has been used with an offset making sure the resulting address is beyond the program break.
(or smth like this)
This caused issues in purecap because sbrk is now disabled, and
Maybe getting the musl reference in a separate sentence, as currently it somewhat reads bit off:
This causes issues for purecap where brk is being disabled, as well as plain aarch64 due to buggy sbrk implementation in musl (ver. 1.5)
in compat with the 1.5 verrsion of musl because of a bug in the sbrk implementation.
Instead, don't rely on sbrk at all and mmap a page, unmap it and use this pointer in order to get an unmapped area of memory, generating an ENOMEM in compat and purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/msync/msync03.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/testcases/kernel/syscalls/msync/msync03.c b/testcases/kernel/syscalls/msync/msync03.c index 8fa62e57a..6d59c9d26 100644 --- a/testcases/kernel/syscalls/msync/msync03.c +++ b/testcases/kernel/syscalls/msync/msync03.c @@ -130,8 +130,17 @@ static void setup(void) SAFE_GETRLIMIT(NULL, RLIMIT_DATA, &rl); addr3 = (char *)(uintptr_t)rl.rlim_max;
- /* memory pointed to by addr4 was not mapped */
- addr4 = sbrk(0) + (4 * page_sz);
- /* memory pointed to by addr4 is not mapped */
What you could potentially do is to increase the mmap size few lines above and then get addr4 to point to an area that can be safely unmapped - you would avoid extra mmap call, so smth like:
addr1 = SAFE_MMAP(cleanup, 0, page_sz * 2, .... ... addr4 = addr + page_sz; SAFE_MUNMAP(addr4, page_size);
- addr4 = mmap(NULL, page_sz, PROT_NONE, MAP_PRIVATE, 0, 0);
- if (addr4 == MAP_FAILED) {
addr4 = NULL;- } else {
if (!munmap(addr4, page_sz))tst_resm(TFAIL, "Could not setup test, munmap failed :\n%s",strerror(errno));
Otherwise, should the test stop here instead ? It will continue and it will report a failure either way.
--- BR B.
- }
- tst_resm(TINFO, "addr4 : %p", addr4);
} static void msync_verify(struct test_case_t *tc) -- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
On 12/12/2022 16:58, Beata Michalska wrote:
Hi Teo,
On Mon, Dec 12, 2022 at 02:59:42PM +0000, Teo Couprie Diaz wrote:
In msync03, sbrk(0) was used and offset to get a pointer to an unmapped area.
How about rewording this a bit with smth along the lines of:
In msync03 testcase, in order to get unmapped memory area, sbrk has been used with an offset making sure the resulting address is beyond the program break.
(or smth like this)
That is much more clear indeed, I will reword along those lines.
This caused issues in purecap because sbrk is now disabled, and
Maybe getting the musl reference in a separate sentence, as currently it somewhat reads bit off:
This causes issues for purecap where brk is being disabled, as well as plain aarch64 due to buggy sbrk implementation in musl (ver. 1.5)
I get what you mean, and it explains the issue better.
in compat with the 1.5 verrsion of musl because of a bug in the sbrk implementation.
Instead, don't rely on sbrk at all and mmap a page, unmap it and use this pointer in order to get an unmapped area of memory, generating an ENOMEM in compat and purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/msync/msync03.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/testcases/kernel/syscalls/msync/msync03.c b/testcases/kernel/syscalls/msync/msync03.c index 8fa62e57a..6d59c9d26 100644 --- a/testcases/kernel/syscalls/msync/msync03.c +++ b/testcases/kernel/syscalls/msync/msync03.c @@ -130,8 +130,17 @@ static void setup(void) SAFE_GETRLIMIT(NULL, RLIMIT_DATA, &rl); addr3 = (char *)(uintptr_t)rl.rlim_max;
- /* memory pointed to by addr4 was not mapped */
- addr4 = sbrk(0) + (4 * page_sz);
- /* memory pointed to by addr4 is not mapped */
What you could potentially do is to increase the mmap size few lines above and then get addr4 to point to an area that can be safely unmapped - you would avoid extra mmap call, so smth like:
addr1 = SAFE_MMAP(cleanup, 0, page_sz * 2, .... ... addr4 = addr + page_sz; SAFE_MUNMAP(addr4, page_size);
Good point, I didn't think of that ! I'll do some testing just in case but it sounds good.
- addr4 = mmap(NULL, page_sz, PROT_NONE, MAP_PRIVATE, 0, 0);
- if (addr4 == MAP_FAILED) {
addr4 = NULL;- } else {
if (!munmap(addr4, page_sz))tst_resm(TFAIL, "Could not setup test, munmap failed :\n%s",strerror(errno));Otherwise, should the test stop here instead ? It will continue and it will report a failure either way.
I hesitated either way, so if you feel that a failure later is acceptable I'm ok letting it go through here. (If it's needed with the SAFE_MUNMAP, I haven't looked at it yet)
BR B.
Thanks for the review ! Regards, Téo
- }
- tst_resm(TINFO, "addr4 : %p", addr4); }
static void msync_verify(struct test_case_t *tc) -- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
On Mon, Dec 12, 2022 at 06:06:23PM +0000, Teo Couprie Diaz wrote:
On 12/12/2022 16:58, Beata Michalska wrote:
Hi Teo,
On Mon, Dec 12, 2022 at 02:59:42PM +0000, Teo Couprie Diaz wrote:
In msync03, sbrk(0) was used and offset to get a pointer to an unmapped area.
How about rewording this a bit with smth along the lines of:
In msync03 testcase, in order to get unmapped memory area, sbrk has been used with an offset making sure the resulting address is beyond the program break.
(or smth like this)
That is much more clear indeed, I will reword along those lines.
This caused issues in purecap because sbrk is now disabled, and
Maybe getting the musl reference in a separate sentence, as currently it somewhat reads bit off:
This causes issues for purecap where brk is being disabled, as well as plain aarch64 due to buggy sbrk implementation in musl (ver. 1.5)
I get what you mean, and it explains the issue better.
Please feel free to reword it however you see fit - those are just my suggestions.
in compat with the 1.5 verrsion of musl because of a bug in the sbrk implementation.
Instead, don't rely on sbrk at all and mmap a page, unmap it and use this pointer in order to get an unmapped area of memory, generating an ENOMEM in compat and purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/msync/msync03.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/testcases/kernel/syscalls/msync/msync03.c b/testcases/kernel/syscalls/msync/msync03.c index 8fa62e57a..6d59c9d26 100644 --- a/testcases/kernel/syscalls/msync/msync03.c +++ b/testcases/kernel/syscalls/msync/msync03.c @@ -130,8 +130,17 @@ static void setup(void) SAFE_GETRLIMIT(NULL, RLIMIT_DATA, &rl); addr3 = (char *)(uintptr_t)rl.rlim_max;
- /* memory pointed to by addr4 was not mapped */
- addr4 = sbrk(0) + (4 * page_sz);
- /* memory pointed to by addr4 is not mapped */
What you could potentially do is to increase the mmap size few lines above and then get addr4 to point to an area that can be safely unmapped - you would avoid extra mmap call, so smth like:
addr1 = SAFE_MMAP(cleanup, 0, page_sz * 2, .... ... addr4 = addr + page_sz; SAFE_MUNMAP(addr4, page_size);
Good point, I didn't think of that ! I'll do some testing just in case but it sounds good.
- addr4 = mmap(NULL, page_sz, PROT_NONE, MAP_PRIVATE, 0, 0);
- if (addr4 == MAP_FAILED) {
addr4 = NULL;- } else {
if (!munmap(addr4, page_sz))tst_resm(TFAIL, "Could not setup test, munmap failed :\n%s",strerror(errno));Otherwise, should the test stop here instead ? It will continue and it will report a failure either way.
I hesitated either way, so if you feel that a failure later is acceptable I'm ok letting it go through here. (If it's needed with the SAFE_MUNMAP, I haven't looked at it yet)
So SAFE_MUNMAP will stop the test here if an error occurs. But I have just noticed that the test itself is using tst_resm to simply report a result without stopping the test itself, so maybe sticking to that approach is more suitable. No strong opinion, as long as one can easily determine the reason for failure if munmap fails.
--- BR B.
BR B.
Thanks for the review ! Regards, Téo
- }
- tst_resm(TINFO, "addr4 : %p", addr4); } static void msync_verify(struct test_case_t *tc)
-- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
On 12/12/2022 20:11, Beata Michalska wrote:
On Mon, Dec 12, 2022 at 06:06:23PM +0000, Teo Couprie Diaz wrote:
On 12/12/2022 16:58, Beata Michalska wrote:
Hi Teo,
On Mon, Dec 12, 2022 at 02:59:42PM +0000, Teo Couprie Diaz wrote:
In msync03, sbrk(0) was used and offset to get a pointer to an unmapped area.
How about rewording this a bit with smth along the lines of:
In msync03 testcase, in order to get unmapped memory area, sbrk has been used with an offset making sure the resulting address is beyond the program break.
(or smth like this)
That is much more clear indeed, I will reword along those lines.
This caused issues in purecap because sbrk is now disabled, and
Maybe getting the musl reference in a separate sentence, as currently it somewhat reads bit off:
This causes issues for purecap where brk is being disabled, as well as plain aarch64 due to buggy sbrk implementation in musl (ver. 1.5)
I get what you mean, and it explains the issue better.
Please feel free to reword it however you see fit - those are just my suggestions.
No worries, I have done so in the v2 !
in compat with the 1.5 verrsion of musl because of a bug in the sbrk implementation.
Instead, don't rely on sbrk at all and mmap a page, unmap it and use this pointer in order to get an unmapped area of memory, generating an ENOMEM in compat and purecap.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
testcases/kernel/syscalls/msync/msync03.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/testcases/kernel/syscalls/msync/msync03.c b/testcases/kernel/syscalls/msync/msync03.c index 8fa62e57a..6d59c9d26 100644 --- a/testcases/kernel/syscalls/msync/msync03.c +++ b/testcases/kernel/syscalls/msync/msync03.c @@ -130,8 +130,17 @@ static void setup(void) SAFE_GETRLIMIT(NULL, RLIMIT_DATA, &rl); addr3 = (char *)(uintptr_t)rl.rlim_max;
- /* memory pointed to by addr4 was not mapped */
- addr4 = sbrk(0) + (4 * page_sz);
- /* memory pointed to by addr4 is not mapped */
What you could potentially do is to increase the mmap size few lines above and then get addr4 to point to an area that can be safely unmapped - you would avoid extra mmap call, so smth like:
addr1 = SAFE_MMAP(cleanup, 0, page_sz * 2, .... ... addr4 = addr + page_sz; SAFE_MUNMAP(addr4, page_size);
Good point, I didn't think of that ! I'll do some testing just in case but it sounds good.
- addr4 = mmap(NULL, page_sz, PROT_NONE, MAP_PRIVATE, 0, 0);
- if (addr4 == MAP_FAILED) {
addr4 = NULL;- } else {
if (!munmap(addr4, page_sz))tst_resm(TFAIL, "Could not setup test, munmap failed :\n%s",strerror(errno));Otherwise, should the test stop here instead ? It will continue and it will report a failure either way.
I hesitated either way, so if you feel that a failure later is acceptable I'm ok letting it go through here. (If it's needed with the SAFE_MUNMAP, I haven't looked at it yet)
So SAFE_MUNMAP will stop the test here if an error occurs. But I have just noticed that the test itself is using tst_resm to simply report a result without stopping the test itself, so maybe sticking to that approach is more suitable. No strong opinion, as long as one can easily determine the reason for failure if munmap fails.
I think SAFE_MUNMAP is good, further the test will already abort early if there is an issue with the SAFE_{OPEN,WRITE,MMAP} that are used earlier in the setup. So I think it's okay if we do the same here.
This is what I have done in the v2. There's a cleanup callback as this test uses the old macros, but I believe we don't need to add anything more for the munmap failure case. (For example, vma04 uses the old SAFE_MUNMAP as well and has an empty cleanup() )
Best regards, Téo
BR B.
BR B.
Thanks for the review ! Regards, Téo
- }
- tst_resm(TINFO, "addr4 : %p", addr4); } static void msync_verify(struct test_case_t *tc)
-- 2.25.1
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
linux-morello-ltp@op-lists.linaro.org
-
Beata Michalska -
Teo Couprie Diaz