Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

Direct usage of brk is discouraged in favor of using malloc and

s/and/as maybe?

it was removed from POSIX in POSIX.1-2001. In particular, the musl libc always returns -ENOMEM for brk which means that the LTP tests fail when calling the kernel directly would pass.

I find this sentence a bit confusing, maybe something along this lines would be more clear:

In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.

sbrk has the same issues so this patch reworks the brk tests slightly to only use direct brk syscalls.

Do you mean that you rewrite the sbrk calls to use the brk syscall instead? If so, I would just say that given that sbrk might have the same issues, you removed the dependency on it. Also, I would make it clear why tst_syscall(__NR_brk, 0) can replace sbrk(0). Up to you!

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

As per internal discussion I believe this patch could be proposed upstream once it is in a good shape. It would fix the brk tests for musl-based systems like Alpine where they currently fail. Note that sbrk is not fixed by this patch as it is fully implemented in the libc: there is no syscall.

testcases/kernel/syscalls/brk/brk01.c | 22 ++++++++++++++++++---- testcases/kernel/syscalls/brk/brk02.c | 17 ++++++++++------- 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..c16b46eaa 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c

Missing: #include "lapi/syscalls.h" I think

@@ -16,7 +16,15 @@ void verify_brk(void) size_t inc = getpagesize() * 2 - 1; unsigned int i;

• cur_brk = (uintptr_t)sbrk(0);

• long ret = tst_syscall(__NR_brk, 0);

Is there any need to have ret? I suggest keeping cur_brk.

• if (ret < 0)
• {

Open brace should be on the previous line. Also below.

• tst_res(TFAIL, "Failed to get current break"); > +		return;
  

• }

I am not sure that the result of the brk syscalls can ever be less than 0. The Linux syscall returns the new program break on success, while on failure, the syscall returns the current break. It should always fail if you call tst_syscall(__NR_brk, 0), so it should always give the current program break. I believe this is what Musl also does for sbrk(0). Therefore, there is no need to have this block. Also below.

• cur_brk = (uintptr_t)ret; > for (i = 0; i < 33; i++) { switch (i % 3) {

@@ -31,11 +39,17 @@ void verify_brk(void) break; }

• TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()");
  

• if (!TST_PASS)
  

• tst_syscall(__NR_brk, new_brk);
  

In the original code, if brk((void *)new_brk), "brk()") fails, the test prints a TFAIL message and ends. This doesn't happen anymore. Just keep the TST_EXP_PASS_SILENT and only replace the brk(..) with tst_syscall(__NR_brk, ...).

• ret = tst_syscall(__NR_brk, 0);
  

• if (ret < 0)
  

• > +			tst_res(TFAIL, "Failed to get current break");
  return;
  

• }
  

• cur_brk = (uintptr_t)sbrk(0);
  

• cur_brk = (uintptr_t)ret;
  

if (cur_brk != new_brk) { tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..19514febb 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,27 @@ #include <unistd.h> #include <sys/mman.h> #include "tst_test.h" +#include "lapi/syscalls.h" void brk_down_vmas(void) {

• void *brk_addr = sbrk(0);

• long ret = tst_syscall(__NR_brk, 0);

I suggest keeping brk_addr here as well. No need to store the result into a long that you cast to void* afterwards. You can change the brk_addr's type to uintptr_t though if you want.

• if (brk_addr == (void *) -1)

• tst_brk(TBROK, "sbrk() failed");
  

• if (ret < 0)

• tst_brk(TBROK, "Failed to get current break");
  

Same as above, tst_syscall(__NR_brk, 0) never returns below 0, so no need to check it.

Good patch overall, only a few nits! Make sure to verify the LTP patches against checkpatch as well.

• void *brk_addr = (void *) ret;

unsigned long page_size = getpagesize(); void *addr = brk_addr + page_size;

• if (brk(addr)) {

• if (tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); return; }

addr += page_size;

• if (brk(addr)) {

• if (tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size"); return; }

@@ -42,12 +45,12 @@ void brk_down_vmas(void) } addr += page_size;

• if (brk(addr)) {

• if (tst_syscall(__NR_brk, addr) < addr) { tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect"); return; }

• if (brk(brk_addr)) {

• if (tst_syscall(__NR_brk, brk_addr) != brk_addr) { tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address"); return; }

On 18-10-2022 13:49, Teo Couprie Diaz wrote:

On 18/10/2022 13:07, Tudor Cretu wrote:

...

Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

Direct usage of brk is discouraged in favor of using malloc and

s/and/as maybe?

I guess it might be a consequence of the removal from the standard, not sure ? I can replace it if you feel that's better.

No preference really.

...

...

it was removed from POSIX in POSIX.1-2001. In particular, the musl libc always returns -ENOMEM for brk which means that the LTP tests fail when calling the kernel directly would pass.

I find this sentence a bit confusing, maybe something along this lines would be more clear:

In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.

That's clearer indeed, thanks !

...

...

sbrk has the same issues so this patch reworks the brk tests slightly to only use direct brk syscalls.

Do you mean that you rewrite the sbrk calls to use the brk syscall instead? If so, I would just say that given that sbrk might have the same issues, you removed the dependency on it. Also, I would make it clear why tst_syscall(__NR_brk, 0) can replace sbrk(0). Up to you!

Exactly. I can try to reword it to make it clearer as you suggest. Explaining why it can replace sbrk is a great point.

...

...

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

As per internal discussion I believe this patch could be proposed upstream once it is in a good shape. It would fix the brk tests for musl-based systems like Alpine where they currently fail. Note that sbrk is not fixed by this patch as it is fully implemented in the libc: there is no syscall.

testcases/kernel/syscalls/brk/brk01.c | 22 ++++++++++++++++++----   testcases/kernel/syscalls/brk/brk02.c | 17 ++++++++++-------   2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..c16b46eaa 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c

Missing: #include "lapi/syscalls.h" I think

Correct, it slipped in the next patch.

...

...

@@ -16,7 +16,15 @@ void verify_brk(void)       size_t inc = getpagesize() * 2 - 1;       unsigned int i;   -    cur_brk = (uintptr_t)sbrk(0); +    long ret = tst_syscall(__NR_brk, 0);

Is there any need to have ret? I suggest keeping cur_brk.

See below.

...

...

+    if (ret < 0) +    {

Open brace should be on the previous line. Also below.

...

+        tst_res(TFAIL, "Failed to get current break"); > + return; +    }

I am not sure that the result of the brk syscalls can ever be less than 0. The Linux syscall returns the new program break on success, while on failure, the syscall returns the current break. It should always fail if you call tst_syscall(__NR_brk, 0), so it should always give the current program break. I believe this is what Musl also does for sbrk(0). Therefore, there is no need to have this block. Also below.

In normal operation it cannot, but in one case it can. See `mm/mmap.c` line 204, in the definition of brk :

if (mmap_write_lock_killable(mm))         return -EINTR;

So there's one case where it returns an errno and thus is negative. Using cur_brk would overflow when hitting the error as it is an unsigned variable, this is why I added the ret variable.

However, I might be missing something regarding errno, syscall errors and EINTR which would make it so it really only returns the previous break or the new one.

Commit dc0ef0df7b6a90892ec41933212ac701152a254c says this:

" ... Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially and allow the current task to die (note that EINTR will never get to the userspace as the task has fatal signal pending). ... "

So, I don't think brk can ever return EINTR to the userspace. And regarding EINTR, the man page for signal(7) says: " If a signal handler is invoked while a system call or library function call is blocked, then either:

* the call is automatically restarted after the signal handler returns; or

* the call fails with the error EINTR.

Which of these two behaviors occurs depends on the interface and whether or not the signal handler was established using the SA_RESTART flag "

In our case, the syscall would just be restarted as we don't have a signal handler without the SA_RESTART flag.

...

...

+    cur_brk = (uintptr_t)ret; >       for (i = 0; i < 33; i++) {           switch (i % 3) { @@ -31,11 +39,17 @@ void verify_brk(void)           break;           }   -        TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); -        if (!TST_PASS) +        tst_syscall(__NR_brk, new_brk);

In the original code, if brk((void *)new_brk), "brk()") fails, the test prints a TFAIL message and ends. This doesn't happen anymore. Just keep the TST_EXP_PASS_SILENT and only replace the brk(..) with tst_syscall(__NR_brk, ...).

According to the documentation :

...

The 'TST_EXP_PASS()' can be used for calls that return -1 on failure

and 0 on

...

success. It will check for the return value and reports failure if

the return

...

value is not equal to 0. The call also sets the 'TST_PASS' variable

to 1 if

...

the call succeeeded.

As tst_syscall returns with the return value of the syscall, I don't believe it would work. (The only difference with the _SILENT version is the announcement or not of the PASS.) I could create a wrapper to replace that but that seems unnecessary as I already check that it hasn't errored and the following test checks that the result is correct.

Indeed, it wouldn't work. Also, doing brk twice seems unnecessary. I think you could assign "cur_break = tst_syscall(__NR_brk, new_brk)" directly and just keep the check for cur_brk != new_brk. Would that work?

...

...

+        ret = tst_syscall(__NR_brk, 0);

+        if (ret < 0) +     > +            tst_res(TFAIL, "Failed to get current break");               return; +        }   -        cur_brk = (uintptr_t)sbrk(0); +        cur_brk = (uintptr_t)ret;             if (cur_brk != new_brk) {               tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..19514febb 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,27 @@   #include <unistd.h>   #include <sys/mman.h>   #include "tst_test.h" +#include "lapi/syscalls.h"     void brk_down_vmas(void)   { -    void *brk_addr = sbrk(0); +    long ret = tst_syscall(__NR_brk, 0);

I suggest keeping brk_addr here as well. No need to store the result into a long that you cast to void* afterwards. You can change the brk_addr's type to uintptr_t though if you want.

Same as above, will change depending on what we decide.

...

...

-    if (brk_addr == (void *) -1) -        tst_brk(TBROK, "sbrk() failed"); +    if (ret < 0) +        tst_brk(TBROK, "Failed to get current break");

Same as above, tst_syscall(__NR_brk, 0) never returns below 0, so no need to check it.

Same as above, will change depending on discussion.

...

Good patch overall, only a few nits! Make sure to verify the LTP patches against checkpatch as well.

Thanks for the review Tudor !

Thanks, Tudor

...

...

+    void *brk_addr = (void *) ret;         unsigned long page_size = getpagesize();       void *addr = brk_addr + page_size;   -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size");           return;       }         addr += page_size; -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size");           return;       } @@ -42,12 +45,12 @@ void brk_down_vmas(void)       }         addr += page_size; -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect");           return;       }   -    if (brk(brk_addr)) { +    if (tst_syscall(__NR_brk, brk_addr) != brk_addr) {           tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address");           return;       }

On 18-10-2022 17:17, Teo Couprie Diaz wrote:

On 18/10/2022 15:18, Tudor Cretu wrote:

...

On 18-10-2022 13:49, Teo Couprie Diaz wrote:

...

On 18/10/2022 13:07, Tudor Cretu wrote:

...

Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

Direct usage of brk is discouraged in favor of using malloc and

s/and/as maybe?

I guess it might be a consequence of the removal from the standard, not sure ? I can replace it if you feel that's better.

No preference really.

...

...

...

it was removed from POSIX in POSIX.1-2001. In particular, the musl libc always returns -ENOMEM for brk which means that the LTP tests fail when calling the kernel directly would pass.

I find this sentence a bit confusing, maybe something along this lines would be more clear:

In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.

That's clearer indeed, thanks !

...

...

sbrk has the same issues so this patch reworks the brk tests slightly to only use direct brk syscalls.

Do you mean that you rewrite the sbrk calls to use the brk syscall instead? If so, I would just say that given that sbrk might have the same issues, you removed the dependency on it. Also, I would make it clear why tst_syscall(__NR_brk, 0) can replace sbrk(0). Up to you!

Exactly. I can try to reword it to make it clearer as you suggest. Explaining why it can replace sbrk is a great point.

...

...

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

As per internal discussion I believe this patch could be proposed upstream once it is in a good shape. It would fix the brk tests for musl-based systems like Alpine where they currently fail. Note that sbrk is not fixed by this patch as it is fully implemented in the libc: there is no syscall.

testcases/kernel/syscalls/brk/brk01.c | 22 ++++++++++++++++++----   testcases/kernel/syscalls/brk/brk02.c | 17 ++++++++++-------   2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..c16b46eaa 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c

Missing: #include "lapi/syscalls.h" I think

Correct, it slipped in the next patch.

...

...

@@ -16,7 +16,15 @@ void verify_brk(void)       size_t inc = getpagesize() * 2 - 1;       unsigned int i;   -    cur_brk = (uintptr_t)sbrk(0); +    long ret = tst_syscall(__NR_brk, 0);

Is there any need to have ret? I suggest keeping cur_brk.

See below.

...

...

+    if (ret < 0) +    {

Open brace should be on the previous line. Also below.

...

+        tst_res(TFAIL, "Failed to get current break"); > + return; +    }

I am not sure that the result of the brk syscalls can ever be less than 0. The Linux syscall returns the new program break on success, while on failure, the syscall returns the current break. It should always fail if you call tst_syscall(__NR_brk, 0), so it should always give the current program break. I believe this is what Musl also does for sbrk(0). Therefore, there is no need to have this block. Also below.

In normal operation it cannot, but in one case it can. See `mm/mmap.c` line 204, in the definition of brk :

if (mmap_write_lock_killable(mm))          return -EINTR;

So there's one case where it returns an errno and thus is negative. Using cur_brk would overflow when hitting the error as it is an unsigned variable, this is why I added the ret variable.

However, I might be missing something regarding errno, syscall errors and EINTR which would make it so it really only returns the previous break or the new one.

Commit dc0ef0df7b6a90892ec41933212ac701152a254c says this:

" ... Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially and allow the current task to die (note that EINTR will never get to the userspace as the task has fatal signal pending). ... "

So, I don't think brk can ever return EINTR to the userspace.

From what I gather of the commit message context it seems to relate to an out-of-memory situation, so I can understand that it would not reach userspace.

...

And regarding EINTR, the man page for signal(7) says: " If a signal handler is invoked while a system call or library function call is blocked, then either:

• the call is automatically restarted after the signal handler

returns; or

• the call fails with the error EINTR.

Which of these two behaviors occurs depends on the interface and whether or not the signal handler was established using the SA_RESTART flag "

In our case, the syscall would just be restarted as we don't have a signal handler without the SA_RESTART flag.

But we don't really have a signal handler _with_ SA_RESTART either, right ? Which I guess means that anyway if there's a signal that pops up and can prevent the mmap lock, we wouldn't have a signal handler for it and it would kill the test anyway ?

I guess what confuses me is that I don't really understand the mechanism that would prevent EINTR to be returned to userspace, given that's the point of the error return and that you need to handle it for other syscalls. My interpretation of the signal man page would be that either you have a signal handler with the flag and it restarts, or you don't and you get EINTR. But I can't find any signal handler that would be set in those tests that would have SA_RESTART, so it would be possible to see it in userspace.

I did see it in strace, so it seems to be restarting and not ending up in userspace, but I feel like I'm missing something to understand.

I think mentioning the signal man page was misleading from my side. I don't think the signal system has anything to do with regards to brk/mmap returning EINTR or not actually. I think that what stops brk/mmap to actually return EINTR is that the only path that reach that point is already on a task with fatal signal pending. At least this is what I understand from tracking the code a bit and this commit: 916633a40370.

You're right related that we don't have a signal handler here. If we don't have a signal handler, then the signal default action is executed, which for some, might be to try to restart the signal.

Hope I clarified a bit all the confusion I created. 🙂

Thanks, Tudor

...

...

...

...

+    cur_brk = (uintptr_t)ret; >       for (i = 0; i < 33; i++) {           switch (i % 3) { @@ -31,11 +39,17 @@ void verify_brk(void)           break;           }   -        TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); -        if (!TST_PASS) +        tst_syscall(__NR_brk, new_brk);

In the original code, if brk((void *)new_brk), "brk()") fails, the test prints a TFAIL message and ends. This doesn't happen anymore. Just keep the TST_EXP_PASS_SILENT and only replace the brk(..) with tst_syscall(__NR_brk, ...).

According to the documentation :

> The 'TST_EXP_PASS()' can be used for calls that return -1 on failure and 0 on  > success. It will check for the return value and reports failure if the return  > value is not equal to 0. The call also sets the 'TST_PASS' variable to 1 if  > the call succeeeded.

As tst_syscall returns with the return value of the syscall, I don't believe it would work. (The only difference with the _SILENT version is the announcement or not of the PASS.) I could create a wrapper to replace that but that seems unnecessary as I already check that it hasn't errored and the following test checks that the result is correct.

Indeed, it wouldn't work. Also, doing brk twice seems unnecessary. I think you could assign "cur_break = tst_syscall(__NR_brk, new_brk)" directly and just keep the check for cur_brk != new_brk. Would that work?

I believe it would indeed, I will make the changes. I tried to stay close to the original code which is why it is weirdly duplicated.

...

...

...

...

+        ret = tst_syscall(__NR_brk, 0);

+        if (ret < 0) +     > +            tst_res(TFAIL, "Failed to get current break");               return; +        }   -        cur_brk = (uintptr_t)sbrk(0); +        cur_brk = (uintptr_t)ret;             if (cur_brk != new_brk) {               tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..19514febb 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,27 @@   #include <unistd.h>   #include <sys/mman.h>   #include "tst_test.h" +#include "lapi/syscalls.h"     void brk_down_vmas(void)   { -    void *brk_addr = sbrk(0); +    long ret = tst_syscall(__NR_brk, 0);

I suggest keeping brk_addr here as well. No need to store the result into a long that you cast to void* afterwards. You can change the brk_addr's type to uintptr_t though if you want.

Same as above, will change depending on what we decide.

...

...

-    if (brk_addr == (void *) -1) -        tst_brk(TBROK, "sbrk() failed"); +    if (ret < 0) +        tst_brk(TBROK, "Failed to get current break");

Same as above, tst_syscall(__NR_brk, 0) never returns below 0, so no need to check it.

Same as above, will change depending on discussion.

...

Good patch overall, only a few nits! Make sure to verify the LTP patches against checkpatch as well.

Thanks for the review Tudor !

Thanks, Tudor

Thanks, Téo

...

...

...

...

+    void *brk_addr = (void *) ret;         unsigned long page_size = getpagesize();       void *addr = brk_addr + page_size;   -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size");           return;       }         addr += page_size; -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size");           return;       } @@ -42,12 +45,12 @@ void brk_down_vmas(void)       }         addr += page_size; -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect");           return;       }   -    if (brk(brk_addr)) { +    if (tst_syscall(__NR_brk, brk_addr) != brk_addr) {           tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address");           return;       }

On Tue, Oct 18, 2022 at 05:17:37PM +0100, Teo Couprie Diaz wrote:

On 18/10/2022 15:18, Tudor Cretu wrote:

...

On 18-10-2022 13:49, Teo Couprie Diaz wrote:

...

On 18/10/2022 13:07, Tudor Cretu wrote:

...

Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

Direct usage of brk is discouraged in favor of using malloc and

s/and/as maybe?

I guess it might be a consequence of the removal from the standard, not sure ? I can replace it if you feel that's better.

No preference really.

...

...

...

it was removed from POSIX in POSIX.1-2001. In particular, the musl libc always returns -ENOMEM for brk which means that the LTP tests fail when calling the kernel directly would pass.

I find this sentence a bit confusing, maybe something along this lines would be more clear:

In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.

That's clearer indeed, thanks !

...

...

sbrk has the same issues so this patch reworks the brk tests slightly to only use direct brk syscalls.

Do you mean that you rewrite the sbrk calls to use the brk syscall instead? If so, I would just say that given that sbrk might have the same issues, you removed the dependency on it. Also, I would make it clear why tst_syscall(__NR_brk, 0) can replace sbrk(0). Up to you!

Exactly. I can try to reword it to make it clearer as you suggest. Explaining why it can replace sbrk is a great point.

...

...

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

As per internal discussion I believe this patch could be proposed upstream once it is in a good shape. It would fix the brk tests for musl-based systems like Alpine where they currently fail. Note that sbrk is not fixed by this patch as it is fully implemented in the libc: there is no syscall.

testcases/kernel/syscalls/brk/brk01.c | 22 ++++++++++++++++++----   testcases/kernel/syscalls/brk/brk02.c | 17 ++++++++++-------   2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index 5f482b943..c16b46eaa 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c

Missing: #include "lapi/syscalls.h" I think

Correct, it slipped in the next patch.

...

...

@@ -16,7 +16,15 @@ void verify_brk(void)       size_t inc = getpagesize() * 2 - 1;       unsigned int i;   -    cur_brk = (uintptr_t)sbrk(0); +    long ret = tst_syscall(__NR_brk, 0);

Is there any need to have ret? I suggest keeping cur_brk.

See below.

A side note: tst_syscall is actually keeping the syscall return value in an integer type and this is what tst_syscall will provide as a result for whichever local variable you will use (though I do opt for getting rid of the intermediate one) - see tst_ret. Either way this will be a subject to implicit convertion which might not be what you need when operating on addresses.

...

...

...

...

+    if (ret < 0) +    {

Open brace should be on the previous line. Also below.

...

+        tst_res(TFAIL, "Failed to get current break"); > + return; +    }

I am not sure that the result of the brk syscalls can ever be less than 0. The Linux syscall returns the new program break on success, while on failure, the syscall returns the current break. It should always fail if you call tst_syscall(__NR_brk, 0), so it should always give the current program break. I believe this is what Musl also does for sbrk(0). Therefore, there is no need to have this block. Also below.

In normal operation it cannot, but in one case it can. See `mm/mmap.c` line 204, in the definition of brk :

if (mmap_write_lock_killable(mm))          return -EINTR;

So there's one case where it returns an errno and thus is negative. Using cur_brk would overflow when hitting the error as it is an unsigned variable, this is why I added the ret variable.

However, I might be missing something regarding errno, syscall errors and EINTR which would make it so it really only returns the previous break or the new one.

Commit dc0ef0df7b6a90892ec41933212ac701152a254c says this:

" ... Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially and allow the current task to die (note that EINTR will never get to the userspace as the task has fatal signal pending). ... "

So, I don't think brk can ever return EINTR to the userspace.

From what I gather of the commit message context it seems to relate to an out-of-memory situation, so I can understand that it would not reach userspace.

I guess this has been sorted out later donw the thread: just to note on the mm locking and the -EINTR : this will happen when the thread as pending fatal signals, in tis case SIGKILL, so it is already a game over for that thread: no handling or blocking for that one.

...

And regarding EINTR, the man page for signal(7) says: " If a signal handler is invoked while a system call or library function call is blocked, then either:

• the call is automatically restarted after the signal handler

returns; or

• the call fails with the error EINTR.

Which of these two behaviors occurs depends on the interface and whether or not the signal handler was established using the SA_RESTART flag "

In our case, the syscall would just be restarted as we don't have a signal handler without the SA_RESTART flag.

But we don't really have a signal handler _with_ SA_RESTART either, right ? Which I guess means that anyway if there's a signal that pops up and can prevent the mmap lock, we wouldn't have a signal handler for it and it would kill the test anyway ?

I guess what confuses me is that I don't really understand the mechanism that would prevent EINTR to be returned to userspace, given that's the point of the error return and that you need to handle it for other syscalls. My interpretation of the signal man page would be that either you have a signal handler with the flag and it restarts, or you don't and you get EINTR. But I can't find any signal handler that would be set in those tests that would have SA_RESTART, so it would be possible to see it in userspace.

I did see it in strace, so it seems to be restarting and not ending up in userspace, but I feel like I'm missing something to understand.

...

...

...

...

+    cur_brk = (uintptr_t)ret; >       for (i = 0; i < 33; i++) {           switch (i % 3) { @@ -31,11 +39,17 @@ void verify_brk(void)           break;           }   -        TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); -        if (!TST_PASS) +        tst_syscall(__NR_brk, new_brk);

In the original code, if brk((void *)new_brk), "brk()") fails, the test prints a TFAIL message and ends. This doesn't happen anymore. Just keep the TST_EXP_PASS_SILENT and only replace the brk(..) with tst_syscall(__NR_brk, ...).

According to the documentation :

> The 'TST_EXP_PASS()' can be used for calls that return -1 on failure and 0 on  > success. It will check for the return value and reports failure if the return  > value is not equal to 0. The call also sets the 'TST_PASS' variable to 1 if  > the call succeeeded.

As tst_syscall returns with the return value of the syscall, I don't believe it would work. (The only difference with the _SILENT version is the announcement or not of the PASS.) I could create a wrapper to replace that but that seems unnecessary as I already check that it hasn't errored and the following test checks that the result is correct.

Indeed, it wouldn't work. Also, doing brk twice seems unnecessary. I think you could assign "cur_break = tst_syscall(__NR_brk, new_brk)" directly and just keep the check for cur_brk != new_brk. Would that work?

I believe it would indeed, I will make the changes. I tried to stay close to the original code which is why it is weirdly duplicated.

Alternatively you could use either TESTPTR and check 'new_brk' against TST_RET_PTR, or TST_EXP_VAL_SILENT (also against the 'new_brk'). Or just compare 'new_brk' against 'cur_brk' as suggested by Tudor.

--- BR B.

...

...

...

...

+        ret = tst_syscall(__NR_brk, 0);

+        if (ret < 0) +     > +            tst_res(TFAIL, "Failed to get current break");               return; +        }   -        cur_brk = (uintptr_t)sbrk(0); +        cur_brk = (uintptr_t)ret;             if (cur_brk != new_brk) {               tst_res(TFAIL, diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c index 11e803cb4..19514febb 100644 --- a/testcases/kernel/syscalls/brk/brk02.c +++ b/testcases/kernel/syscalls/brk/brk02.c @@ -14,24 +14,27 @@   #include <unistd.h>   #include <sys/mman.h>   #include "tst_test.h" +#include "lapi/syscalls.h"     void brk_down_vmas(void)   { -    void *brk_addr = sbrk(0); +    long ret = tst_syscall(__NR_brk, 0);

I suggest keeping brk_addr here as well. No need to store the result into a long that you cast to void* afterwards. You can change the brk_addr's type to uintptr_t though if you want.

Same as above, will change depending on what we decide.

...

...

-    if (brk_addr == (void *) -1) -        tst_brk(TBROK, "sbrk() failed"); +    if (ret < 0) +        tst_brk(TBROK, "Failed to get current break");

Same as above, tst_syscall(__NR_brk, 0) never returns below 0, so no need to check it.

Same as above, will change depending on discussion.

...

Good patch overall, only a few nits! Make sure to verify the LTP patches against checkpatch as well.

Thanks for the review Tudor !

Thanks, Tudor

Thanks, Téo

...

...

...

...

+    void *brk_addr = (void *) ret;         unsigned long page_size = getpagesize();       void *addr = brk_addr + page_size;   -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size");           return;       }         addr += page_size; -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x page size");           return;       } @@ -42,12 +45,12 @@ void brk_down_vmas(void)       }         addr += page_size; -    if (brk(addr)) { +    if (tst_syscall(__NR_brk, addr) < addr) {           tst_res(TFAIL | TERRNO, "Cannot expand brk() after mprotect");           return;       }   -    if (brk(brk_addr)) { +    if (tst_syscall(__NR_brk, brk_addr) != brk_addr) {           tst_res(TFAIL | TERRNO, "Cannot restore brk() to start address");           return;       }

---

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 Wed, Oct 19, 2022 at 04:39:20PM +0100, Teo Couprie Diaz wrote:

On 19/10/2022 15:31, Beata Michalska wrote:

...

On Tue, Oct 18, 2022 at 05:17:37PM +0100, Teo Couprie Diaz wrote:

...

On 18/10/2022 15:18, Tudor Cretu wrote:

...

On 18-10-2022 13:49, Teo Couprie Diaz wrote:

...

On 18/10/2022 13:07, Tudor Cretu wrote:

...

Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote: > Direct usage of brk is discouraged in favor of using malloc and s/and/as maybe?

I guess it might be a consequence of the removal from the standard, not sure ? I can replace it if you feel that's better.

No preference really.

...

...

> it was removed from POSIX in POSIX.1-2001. > In particular, the musl libc always returns -ENOMEM for brk > which means > that the LTP tests fail when calling the kernel directly would pass. I find this sentence a bit confusing, maybe something along this lines would be more clear:

� In particular, the musl libc's brk always returns -ENOMEM which causes the LTP tests to fail. Invoking the syscall directly allows them to pass.

That's clearer indeed, thanks !

...

> sbrk has the same issues so this patch reworks the brk tests > slightly to > only use direct brk syscalls. Do you mean that you rewrite the sbrk calls to use the brk syscall instead? If so, I would just say that given that sbrk might have the same issues, you removed the dependency on it. Also, I would make it clear why tst_syscall(__NR_brk, 0) can replace sbrk(0). Up to you!

Exactly. I can try to reword it to make it clearer as you suggest. Explaining why it can replace sbrk is a great point.

...

> Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com > --- > As per internal discussion I believe this patch could be > proposed upstream > once it is in a good shape. It would fix the brk tests for musl-based > systems like Alpine where they currently fail. > Note that sbrk is not fixed by this patch as it is fully > implemented in > the libc: there is no syscall. > > � testcases/kernel/syscalls/brk/brk01.c | 22 ++++++++++++++++++---- > � testcases/kernel/syscalls/brk/brk02.c | 17 ++++++++++------- > � 2 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/testcases/kernel/syscalls/brk/brk01.c > b/testcases/kernel/syscalls/brk/brk01.c > index 5f482b943..c16b46eaa 100644 > --- a/testcases/kernel/syscalls/brk/brk01.c > +++ b/testcases/kernel/syscalls/brk/brk01.c Missing: #include "lapi/syscalls.h" I think

Correct, it slipped in the next patch.

...

> @@ -16,7 +16,15 @@ void verify_brk(void) > ����� size_t inc = getpagesize() * 2 - 1; > ����� unsigned int i; > � -��� cur_brk = (uintptr_t)sbrk(0); > +��� long ret = tst_syscall(__NR_brk, 0); > + Is there any need to have ret? I suggest keeping cur_brk.

See below.

A side note: tst_syscall is actually keeping the syscall return value in an integer type and this is what tst_syscall will provide as a result for whichever local variable you will use (though I do opt for getting rid of the intermediate one) - see tst_ret.

It does, but it seems to be in its own scope and thus inaccessible after the assignment. It doesn't use the shared TST_RET either it seems. Maybe I'm missing something ?

That's a compound statement, where the last expression is supposed to act as a final value of the entire statement (tst_ret in this case), see:

https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

Anyway, as established below and on another reply to the thread : as the EINTR can't reach userspace, there's no point in checking thus no point in the intermediate value !

True, but those are separated issues, or more of: one does not eliminate the other.

--- BR B.

...

Either way this will be a subject to implicit convertion which might not be what you need when operating on addresses.

...

...

...

...

> +��� if (ret < 0) > +��� { Open brace should be on the previous line. Also below.

> +������� tst_res(TFAIL, "Failed to get current break"); > + return; > +��� } I am not sure that the result of the brk syscalls can ever be less than 0. The Linux syscall returns the new program break on success, while on failure, the syscall returns the current break. It should always fail if you call tst_syscall(__NR_brk, 0), so it should always give the current program break. I believe this is what Musl also does for sbrk(0). Therefore, there is no need to have this block. Also below.

In normal operation it cannot, but in one case it can. See `mm/mmap.c` line 204, in the definition of brk :

if (mmap_write_lock_killable(mm)) ��� ���� return -EINTR;

So there's one case where it returns an errno and thus is negative. Using cur_brk would overflow when hitting the error as it is an unsigned variable, this is why I added the ret variable.

However, I might be missing something regarding errno, syscall errors and EINTR which would make it so it really only returns the previous break or the new one.

Commit dc0ef0df7b6a90892ec41933212ac701152a254c says this:

" ... Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially and allow the current task to die (note that EINTR will never get to the userspace as the task has fatal signal pending). ... "

So, I don't think brk can ever return EINTR to the userspace.

From what I gather of the commit message context it seems to relate to an out-of-memory situation, so I can understand that it would not reach userspace.

I guess this has been sorted out later donw the thread: just to note on the mm locking and the -EINTR : this will happen when the thread as pending fatal signals, in tis case SIGKILL, so it is already a game over for that thread: no handling or blocking for that one.

That is what our final understanding was, thanks for confirming !

...

...

...

And regarding EINTR, the man page for signal(7) says: " If a signal handler is invoked while a system call or library function call is blocked, then either:

• the call is automatically restarted after the signal handler

� returns; or

• the call fails with the error EINTR.

Which of these two behaviors occurs depends on the interface and whether or not the signal handler was established using the SA_RESTART flag "

In our case, the syscall would just be restarted as we don't have a signal handler without the SA_RESTART flag.

But we don't really have a signal handler _with_ SA_RESTART either, right ? Which I guess means that anyway if there's a signal that pops up and can prevent the mmap lock, we wouldn't have a signal handler for it and it would kill the test anyway ?

I guess what confuses me is that I don't really understand the mechanism that would prevent EINTR to be returned to userspace, given that's the point of the error return and that you need to handle it for other syscalls. My interpretation of the signal man page would be that either you have a signal handler with the flag and it restarts, or you don't and you get EINTR. But I can't find any signal handler that would be set in those tests that would have SA_RESTART, so it would be possible to see it in userspace.

I did see it in strace, so it seems to be restarting and not ending up in userspace, but I feel like I'm missing something to understand.

...

...

...

> + > +��� cur_brk = (uintptr_t)ret; > > ����� for (i = 0; i < 33; i++) { > ��������� switch (i % 3) { > @@ -31,11 +39,17 @@ void verify_brk(void) > ��������� break; > ��������� } > � -������� TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); > -������� if (!TST_PASS) > +������� tst_syscall(__NR_brk, new_brk); In the original code, if brk((void *)new_brk), "brk()") fails, the test prints a TFAIL message and ends. This doesn't happen anymore. Just keep the TST_EXP_PASS_SILENT and only replace the brk(..) with tst_syscall(__NR_brk, ...).

According to the documentation :

�> The 'TST_EXP_PASS()' can be used for calls that return -1 on failure and 0 on �> success. It will check for the return value and reports failure if the return �> value is not equal to 0. The call also sets the 'TST_PASS' variable to 1 if �> the call succeeeded.

As tst_syscall returns with the return value of the syscall, I don't believe it would work. (The only difference with the _SILENT version is the announcement or not of the PASS.) I could create a wrapper to replace that but that seems unnecessary as I already check that it hasn't errored and the following test checks that the result is correct.

Indeed, it wouldn't work. Also, doing brk twice seems unnecessary. I think you could assign "cur_break = tst_syscall(__NR_brk, new_brk)" directly and just keep the check for cur_brk != new_brk. Would that work?

I believe it would indeed, I will make the changes. I tried to stay close to the original code which is why it is weirdly duplicated.

Alternatively you could use either TESTPTR and check 'new_brk' against TST_RET_PTR, or TST_EXP_VAL_SILENT (also against the 'new_brk'). Or just compare 'new_brk' against 'cur_brk' as suggested by Tudor.

I did prepare a change by just checking 'new_brk' against 'cur_brk' but it might be more proper with what you suggest, I'll give it a look.

...

---

BR B.

Thanks for the comments ! Téo

...

...

...

...

...

> + > +������� ret = tst_syscall(__NR_brk, 0); > + > +������� if (ret < 0) > +���� > +����������� tst_res(TFAIL, "Failed to get current break"); > ������������� return; > +������� } > � -������� cur_brk = (uintptr_t)sbrk(0); > +������� cur_brk = (uintptr_t)ret; > � ��������� if (cur_brk != new_brk) { > ������������� tst_res(TFAIL, > diff --git a/testcases/kernel/syscalls/brk/brk02.c > b/testcases/kernel/syscalls/brk/brk02.c > index 11e803cb4..19514febb 100644 > --- a/testcases/kernel/syscalls/brk/brk02.c > +++ b/testcases/kernel/syscalls/brk/brk02.c > @@ -14,24 +14,27 @@ > � #include <unistd.h> > � #include <sys/mman.h> > � #include "tst_test.h" > +#include "lapi/syscalls.h" > � � void brk_down_vmas(void) > � { > -��� void *brk_addr = sbrk(0); > +��� long ret = tst_syscall(__NR_brk, 0); I suggest keeping brk_addr here as well. No need to store the result into a long that you cast to void* afterwards. You can change the brk_addr's type to uintptr_t though if you want.

Same as above, will change depending on what we decide.

...

> � -��� if (brk_addr == (void *) -1) > -������� tst_brk(TBROK, "sbrk() failed"); > +��� if (ret < 0) > +������� tst_brk(TBROK, "Failed to get current break"); Same as above, tst_syscall(__NR_brk, 0) never returns below 0, so no need to check it.

Same as above, will change depending on discussion.

...

Good patch overall, only a few nits! Make sure to verify the LTP patches against checkpatch as well.

Thanks for the review Tudor !

Thanks, Tudor

Thanks, T�o

...

...

...

> + > +��� void *brk_addr = (void *) ret; > � ����� unsigned long page_size = getpagesize(); > ����� void *addr = brk_addr + page_size; > � -��� if (brk(addr)) { > +��� if (tst_syscall(__NR_brk, addr) < addr) { > ��������� tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); > ��������� return; > ����� } > � ����� addr += page_size; > -��� if (brk(addr)) { > +��� if (tst_syscall(__NR_brk, addr) < addr) { > ��������� tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x > page size"); > ��������� return; > ����� } > @@ -42,12 +45,12 @@ void brk_down_vmas(void) > ����� } > � ����� addr += page_size; > -��� if (brk(addr)) { > +��� if (tst_syscall(__NR_brk, addr) < addr) { > ��������� tst_res(TFAIL | TERRNO, "Cannot expand brk() after > mprotect"); > ��������� return; > ����� } > � -��� if (brk(brk_addr)) { > +��� if (tst_syscall(__NR_brk, brk_addr) != brk_addr) { > ��������� tst_res(TFAIL | TERRNO, "Cannot restore brk() to > start address"); > ��������� return; > ����� }

---

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 Thu, Oct 20, 2022 at 10:31:58AM +0100, Teo Couprie Diaz wrote:

On 19/10/2022 18:16, Beata Michalska wrote:

...

On Wed, Oct 19, 2022 at 04:39:20PM +0100, Teo Couprie Diaz wrote:

...

On 19/10/2022 15:31, Beata Michalska wrote:

...

On Tue, Oct 18, 2022 at 05:17:37PM +0100, Teo Couprie Diaz wrote:

...

On 18/10/2022 15:18, Tudor Cretu wrote:

...

On 18-10-2022 13:49, Teo Couprie Diaz wrote: > On 18/10/2022 13:07, Tudor Cretu wrote: > > > Hi Teo, > > > > On 18-10-2022 09:46, Teo Couprie Diaz wrote: > > > Direct usage of brk is discouraged in favor of using malloc and > > s/and/as maybe? > I guess it might be a consequence of the removal from the standard, > not sure ? I can replace it if you feel that's better. No preference really.

> > > it was removed from POSIX in POSIX.1-2001. > > > In particular, the musl libc always returns -ENOMEM for brk > > > which means > > > that the LTP tests fail when calling the kernel directly would pass. > > I find this sentence a bit confusing, maybe something along this > > lines would be more clear: > > > > � In particular, the musl libc's brk always returns -ENOMEM > > which causes the LTP tests to fail. Invoking the syscall > > directly allows them to pass. > That's clearer indeed, thanks ! > > > sbrk has the same issues so this patch reworks the brk tests > > > slightly to > > > only use direct brk syscalls. > > Do you mean that you rewrite the sbrk calls to use the brk > > syscall instead? If so, I would just say that given that sbrk > > might have the same issues, you removed the dependency on it. > > Also, I would make it clear why tst_syscall(__NR_brk, 0) can > > replace sbrk(0). Up to you! > Exactly. I can try to reword it to make it clearer as you suggest. > Explaining why it can replace sbrk is a great point. > > > Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com > > > --- > > > As per internal discussion I believe this patch could be > > > proposed upstream > > > once it is in a good shape. It would fix the brk tests for musl-based > > > systems like Alpine where they currently fail. > > > Note that sbrk is not fixed by this patch as it is fully > > > implemented in > > > the libc: there is no syscall. > > > > > > � testcases/kernel/syscalls/brk/brk01.c | 22 ++++++++++++++++++---- > > > � testcases/kernel/syscalls/brk/brk02.c | 17 ++++++++++------- > > > � 2 files changed, 28 insertions(+), 11 deletions(-) > > > > > > diff --git a/testcases/kernel/syscalls/brk/brk01.c > > > b/testcases/kernel/syscalls/brk/brk01.c > > > index 5f482b943..c16b46eaa 100644 > > > --- a/testcases/kernel/syscalls/brk/brk01.c > > > +++ b/testcases/kernel/syscalls/brk/brk01.c > > Missing: #include "lapi/syscalls.h" I think > Correct, it slipped in the next patch. > > > @@ -16,7 +16,15 @@ void verify_brk(void) > > > ����� size_t inc = getpagesize() * 2 - 1; > > > ����� unsigned int i; > > > � -��� cur_brk = (uintptr_t)sbrk(0); > > > +��� long ret = tst_syscall(__NR_brk, 0); > > > + > > Is there any need to have ret? I suggest keeping cur_brk. > See below.

A side note: tst_syscall is actually keeping the syscall return value in an integer type and this is what tst_syscall will provide as a result for whichever local variable you will use (though I do opt for getting rid of the intermediate one) - see tst_ret.

It does, but it seems to be in its own scope and thus inaccessible after the assignment. It doesn't use the shared TST_RET either it seems. Maybe I'm missing something ?

That's a compound statement, where the last expression is supposed to act as a final value of the entire statement (tst_ret in this case), see:

https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

It is indeed, that's the way I understood it but it's nice to put a name on it, thanks. I think we had a misunderstanding : I thought you were suggesting a different way of getting the return value from tst_syscall, but reading your first message again it seems you were just pointing out the details of tst_syscall in case I didn't have a look. Is that correct, or am I still missing what you want to convey ?

I actually was, suggesting modifying tst_syscall cause it is problematic, thus me pointing it out - I should have been more explicit about it. I do appreciate though that making sure that nothing gets broken while changing tst_syscall might be bit tedious.

--- BR B.

...

...

Anyway, as established below and on another reply to the thread : as the EINTR can't reach userspace, there's no point in checking thus no point in the intermediate value !

True, but those are separated issues, or more of: one does not eliminate the other.

---

BR B.

Thanks, Téo

...

...

...

Either way this will be a subject to implicit convertion which might not be what you need when operating on addresses.

...

...

> > > +��� if (ret < 0) > > > +��� { > > Open brace should be on the previous line. Also below. > > > > > +������� tst_res(TFAIL, "Failed to get current break"); > + return; > > > +��� } > > I am not sure that the result of the brk syscalls can ever be > > less than 0. The Linux syscall returns the new program break on > > success, while on failure, the syscall returns the current > > break. It should always fail if you call tst_syscall(__NR_brk, > > 0), so it should always give the current program break. I > > believe this is what Musl also does for sbrk(0). Therefore, > > there is no need to have this block. Also below. > In normal operation it cannot, but in one case it can. See > `mm/mmap.c` line 204, in the definition of brk : > > if (mmap_write_lock_killable(mm)) > ��� ���� return -EINTR; > > So there's one case where it returns an errno and thus is negative. > Using cur_brk would overflow when hitting the error as it is an > unsigned variable, this is why I added the ret variable. > > However, I might be missing something regarding errno, syscall > errors and EINTR which would make it so it really only returns the > previous break or the new one. Commit dc0ef0df7b6a90892ec41933212ac701152a254c says this:

" ... Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially and allow the current task to die (note that EINTR will never get to the userspace as the task has fatal signal pending). ... "

So, I don't think brk can ever return EINTR to the userspace.

From what I gather of the commit message context it seems to relate to an out-of-memory situation, so I can understand that it would not reach userspace.

I guess this has been sorted out later donw the thread: just to note on the mm locking and the -EINTR : this will happen when the thread as pending fatal signals, in tis case SIGKILL, so it is already a game over for that thread: no handling or blocking for that one.

That is what our final understanding was, thanks for confirming !

...

...

...

And regarding EINTR, the man page for signal(7) says: " If a signal handler is invoked while a system call or library function call is blocked, then either:

• the call is automatically restarted after the signal handler

� returns; or

• the call fails with the error EINTR.

Which of these two behaviors occurs depends on the interface and whether or not the signal handler was established using the SA_RESTART flag "

In our case, the syscall would just be restarted as we don't have a signal handler without the SA_RESTART flag.

But we don't really have a signal handler _with_ SA_RESTART either, right ? Which I guess means that anyway if there's a signal that pops up and can prevent the mmap lock, we wouldn't have a signal handler for it and it would kill the test anyway ?

I guess what confuses me is that I don't really understand the mechanism that would prevent EINTR to be returned to userspace, given that's the point of the error return and that you need to handle it for other syscalls. My interpretation of the signal man page would be that either you have a signal handler with the flag and it restarts, or you don't and you get EINTR. But I can't find any signal handler that would be set in those tests that would have SA_RESTART, so it would be possible to see it in userspace.

I did see it in strace, so it seems to be restarting and not ending up in userspace, but I feel like I'm missing something to understand.

...

> > > + > > > +��� cur_brk = (uintptr_t)ret; > > > > ����� for (i = 0; i < 33; i++) { > > > ��������� switch (i % 3) { > > > @@ -31,11 +39,17 @@ void verify_brk(void) > > > ��������� break; > > > ��������� } > > > � -������� TST_EXP_PASS_SILENT(brk((void *)new_brk), "brk()"); > > > -������� if (!TST_PASS) > > > +������� tst_syscall(__NR_brk, new_brk); > > In the original code, if brk((void *)new_brk), "brk()") fails, > > the test prints a TFAIL message and ends. This doesn't happen > > anymore. Just keep the TST_EXP_PASS_SILENT and only replace the > > brk(..) with tst_syscall(__NR_brk, ...). > According to the documentation : > > �> The 'TST_EXP_PASS()' can be used for calls that return -1 on > failure and 0 on > �> success. It will check for the return value and reports failure > if the return > �> value is not equal to 0. The call also sets the 'TST_PASS' > variable to 1 if > �> the call succeeeded. > > As tst_syscall returns with the return value of the syscall, I don't > believe it would work. (The only difference with the _SILENT version > is the announcement or not of the PASS.) > I could create a wrapper to replace that but that seems unnecessary > as I already check that it hasn't errored and the following test > checks that the result is correct. Indeed, it wouldn't work. Also, doing brk twice seems unnecessary. I think you could assign "cur_break = tst_syscall(__NR_brk, new_brk)" directly and just keep the check for cur_brk != new_brk. Would that work?

I believe it would indeed, I will make the changes. I tried to stay close to the original code which is why it is weirdly duplicated.

Alternatively you could use either TESTPTR and check 'new_brk' against TST_RET_PTR, or TST_EXP_VAL_SILENT (also against the 'new_brk'). Or just compare 'new_brk' against 'cur_brk' as suggested by Tudor.

I did prepare a change by just checking 'new_brk' against 'cur_brk' but it might be more proper with what you suggest, I'll give it a look.

...

---

BR B.

Thanks for the comments ! T�o

...

...

...

> > > + > > > +������� ret = tst_syscall(__NR_brk, 0); > > > + > > > +������� if (ret < 0) > > > +���� > +����������� tst_res(TFAIL, "Failed to get current break"); > > > ������������� return; > > > +������� } > > > � -������� cur_brk = (uintptr_t)sbrk(0); > > > +������� cur_brk = (uintptr_t)ret; > > > � ��������� if (cur_brk != new_brk) { > > > ������������� tst_res(TFAIL, > > > diff --git a/testcases/kernel/syscalls/brk/brk02.c > > > b/testcases/kernel/syscalls/brk/brk02.c > > > index 11e803cb4..19514febb 100644 > > > --- a/testcases/kernel/syscalls/brk/brk02.c > > > +++ b/testcases/kernel/syscalls/brk/brk02.c > > > @@ -14,24 +14,27 @@ > > > � #include <unistd.h> > > > � #include <sys/mman.h> > > > � #include "tst_test.h" > > > +#include "lapi/syscalls.h" > > > � � void brk_down_vmas(void) > > > � { > > > -��� void *brk_addr = sbrk(0); > > > +��� long ret = tst_syscall(__NR_brk, 0); > > I suggest keeping brk_addr here as well. No need to store the > > result into a long that you cast to void* afterwards. You can > > change the brk_addr's type to uintptr_t though if you want. > Same as above, will change depending on what we decide. > > > � -��� if (brk_addr == (void *) -1) > > > -������� tst_brk(TBROK, "sbrk() failed"); > > > +��� if (ret < 0) > > > +������� tst_brk(TBROK, "Failed to get current break"); > > Same as above, tst_syscall(__NR_brk, 0) never returns below 0, > > so no need to check it. > Same as above, will change depending on discussion. > > > Good patch overall, only a few nits! Make sure to verify the LTP > > patches against checkpatch as well. > Thanks for the review Tudor ! Thanks, Tudor

Thanks, T�o

...

> > > + > > > +��� void *brk_addr = (void *) ret; > > > � ����� unsigned long page_size = getpagesize(); > > > ����� void *addr = brk_addr + page_size; > > > � -��� if (brk(addr)) { > > > +��� if (tst_syscall(__NR_brk, addr) < addr) { > > > ��������� tst_res(TFAIL | TERRNO, "Cannot expand brk() by page size"); > > > ��������� return; > > > ����� } > > > � ����� addr += page_size; > > > -��� if (brk(addr)) { > > > +��� if (tst_syscall(__NR_brk, addr) < addr) { > > > ��������� tst_res(TFAIL | TERRNO, "Cannot expand brk() by 2x > > > page size"); > > > ��������� return; > > > ����� } > > > @@ -42,12 +45,12 @@ void brk_down_vmas(void) > > > ����� } > > > � ����� addr += page_size; > > > -��� if (brk(addr)) { > > > +��� if (tst_syscall(__NR_brk, addr) < addr) { > > > ��������� tst_res(TFAIL | TERRNO, "Cannot expand brk() after > > > mprotect"); > > > ��������� return; > > > ����� } > > > � -��� if (brk(brk_addr)) { > > > +��� if (tst_syscall(__NR_brk, brk_addr) != brk_addr) { > > > ��������� tst_res(TFAIL | TERRNO, "Cannot restore brk() to > > > start address"); > > > ��������� return; > > > ����� }

---

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 25/10/2022 10:53, Tudor Cretu wrote:

On 25-10-2022 08:27, Beata Michalska wrote:

...

On Thu, Oct 20, 2022 at 10:31:58AM +0100, Teo Couprie Diaz wrote:

...

On 19/10/2022 18:16, Beata Michalska wrote:

...

On Wed, Oct 19, 2022 at 04:39:20PM +0100, Teo Couprie Diaz wrote:

...

On 19/10/2022 15:31, Beata Michalska wrote:

...

On Tue, Oct 18, 2022 at 05:17:37PM +0100, Teo Couprie Diaz wrote: > On 18/10/2022 15:18, Tudor Cretu wrote: > >> On 18-10-2022 13:49, Teo Couprie Diaz wrote: >>> On 18/10/2022 13:07, Tudor Cretu wrote: >>> >>>> Hi Teo, >>>> >>>> On 18-10-2022 09:46, Teo Couprie Diaz wrote: >>>>> Direct usage of brk is discouraged in favor of using malloc and >>>> s/and/as maybe? >>> I guess it might be a consequence of the removal from the >>> standard, >>> not sure ? I can replace it if you feel that's better. >> No preference really. >> >>>>> it was removed from POSIX in POSIX.1-2001. >>>>> In particular, the musl libc always returns -ENOMEM for brk >>>>> which means >>>>> that the LTP tests fail when calling the kernel directly >>>>> would pass. >>>> I find this sentence a bit confusing, maybe something along this >>>> lines would be more clear: >>>> >>>> � In particular, the musl libc's brk always returns -ENOMEM >>>> which causes the LTP tests to fail. Invoking the syscall >>>> directly allows them to pass. >>> That's clearer indeed, thanks ! >>>>> sbrk has the same issues so this patch reworks the brk tests >>>>> slightly to >>>>> only use direct brk syscalls. >>>> Do you mean that you rewrite the sbrk calls to use the brk >>>> syscall instead? If so, I would just say that given that sbrk >>>> might have the same issues, you removed the dependency on it. >>>> Also, I would make it clear why tst_syscall(__NR_brk, 0) can >>>> replace sbrk(0). Up to you! >>> Exactly. I can try to reword it to make it clearer as you >>> suggest. >>> Explaining why it can replace sbrk is a great point. >>>>> Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com >>>>> --- >>>>> As per internal discussion I believe this patch could be >>>>> proposed upstream >>>>> once it is in a good shape. It would fix the brk tests for >>>>> musl-based >>>>> systems like Alpine where they currently fail. >>>>> Note that sbrk is not fixed by this patch as it is fully >>>>> implemented in >>>>> the libc: there is no syscall. >>>>> >>>>> � testcases/kernel/syscalls/brk/brk01.c | 22 >>>>> ++++++++++++++++++---- >>>>> � testcases/kernel/syscalls/brk/brk02.c | 17 >>>>> ++++++++++------- >>>>> � 2 files changed, 28 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/testcases/kernel/syscalls/brk/brk01.c >>>>> b/testcases/kernel/syscalls/brk/brk01.c >>>>> index 5f482b943..c16b46eaa 100644 >>>>> --- a/testcases/kernel/syscalls/brk/brk01.c >>>>> +++ b/testcases/kernel/syscalls/brk/brk01.c >>>> Missing: #include "lapi/syscalls.h" I think >>> Correct, it slipped in the next patch. >>>>> @@ -16,7 +16,15 @@ void verify_brk(void) >>>>> ����� size_t inc = getpagesize() * 2 - 1; >>>>> ����� unsigned int i; >>>>> � -��� cur_brk = (uintptr_t)sbrk(0); >>>>> +��� long ret = tst_syscall(__NR_brk, 0); >>>>> + >>>> Is there any need to have ret? I suggest keeping cur_brk. >>> See below. A side note: tst_syscall is actually keeping the syscall return value in an integer type and this is what tst_syscall will provide as a result for whichever local variable you will use (though I do opt for getting rid of the intermediate one) - see tst_ret.

It does, but it seems to be in its own scope and thus inaccessible after the assignment. It doesn't use the shared TST_RET either it seems. Maybe I'm missing something ?

That's a compound statement, where the last expression is supposed to act as a final value of the entire statement (tst_ret in this case), see:

https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

It is indeed, that's the way I understood it but it's nice to put a name on it, thanks. I think we had a misunderstanding : I thought you were suggesting a different way of getting the return value from tst_syscall, but reading your first message again it seems you were just pointing out the details of tst_syscall in case I didn't have a look. Is that correct, or am I still missing what you want to convey ?

I actually was, suggesting modifying  cause it is problematic, thus me pointing it out - I should have been more explicit about it.

Ah, right, I did miss that completely then : I thought you were suggesting for changes in the test rather than in tst_syscall. It makes more sense now.

...

I do appreciate though that making sure that nothing gets broken while changing tst_syscall might be bit tedious.

+1 in favour of making tst_syscall return a long. TEST(...) is already handling long. Also, syscall(...) is returning long. There are several examples in upstream where tst_syscall(__NR_SYSCALL) is used for syscalls that return ssize_t or long. I think there is enough incentive to implement such a change.

It definitely feels weird in the current state, and wrong in some cases.

IIRC, you can invoke the upstream LTP CI with your change to test if it breaks anything. In my opinion it should be sufficient.

I have done some preliminary testing on x86_64 musl and it didn't break anything from the `syscalls` test list. I will look into invoking the upstream CI then, I agree that it ought to be a good check.

Thanks, Tudor

...

---

BR B.

Thanks both, Téo

...

...

...

...

Anyway, as established below and on another reply to the thread : as the EINTR can't reach userspace, there's no point in checking thus no point in the intermediate value !

True, but those are separated issues, or more of: one does not eliminate the other.

---

BR B.

Thanks, T�o

...

...

...

Either way this will be a subject to implicit convertion which might not be what you need when operating on addresses.

>>>>> +��� if (ret < 0) >>>>> +��� { >>>> Open brace should be on the previous line. Also below. >>>> >>>>> +������� tst_res(TFAIL, "Failed to get current >>>>> break"); > + return; >>>>> +��� } >>>> I am not sure that the result of the brk syscalls can ever be >>>> less than 0. The Linux syscall returns the new program break on >>>> success, while on failure, the syscall returns the current >>>> break. It should always fail if you call tst_syscall(__NR_brk, >>>> 0), so it should always give the current program break. I >>>> believe this is what Musl also does for sbrk(0). Therefore, >>>> there is no need to have this block. Also below. >>> In normal operation it cannot, but in one case it can. See >>> `mm/mmap.c` line 204, in the definition of brk : >>> >>> if (mmap_write_lock_killable(mm)) >>> ��� ���� return -EINTR; >>> >>> So there's one case where it returns an errno and thus is >>> negative. >>> Using cur_brk would overflow when hitting the error as it is an >>> unsigned variable, this is why I added the ret variable. >>> >>> However, I might be missing something regarding errno, syscall >>> errors and EINTR which would make it so it really only returns >>> the >>> previous break or the new one. >> Commit dc0ef0df7b6a90892ec41933212ac701152a254c says this: >> >> " >> ... >> Most of the patches are really trivial because the lock is help >> from a >> shallow syscall paths where we can return EINTR trivially and >> allow the >> current task to die (note that EINTR will never get to the >> userspace as >> the task has fatal signal pending). ... >> " >> >> So, I don't think brk can ever return EINTR to the userspace. >    From what I gather of the commit message context it seems to > relate to an > out-of-memory situation, so I can understand that it would not > reach > userspace. I guess this has been sorted out later donw the thread: just to note on the mm locking and the -EINTR : this will happen when the thread as pending fatal signals, in tis case SIGKILL, so it is already a game over for that thread: no handling or blocking for that one.

That is what our final understanding was, thanks for confirming !

...

>> And regarding EINTR, the man page for signal(7) says: >> " >> If a signal handler is invoked while a system call or library >> function >> call is blocked, then either: >> >> * the call is automatically restarted after the signal handler >> � returns; or >> >> * the call fails with the error EINTR. >> >> Which of these two behaviors occurs depends on the interface >> and whether >> or not the signal handler was established using the SA_RESTART >> flag >> " >> >> In our case, the syscall would just be restarted as we don't >> have a >> signal handler without the SA_RESTART flag. > But we don't really have a signal handler _with_ SA_RESTART > either, right ? > Which I guess means that anyway if there's a signal that pops up > and can > prevent the mmap lock, we wouldn't have a signal handler for it > and it would > kill the test anyway ? > > I guess what confuses me is that I don't really understand the > mechanism > that would prevent EINTR to be returned to userspace, given > that's the point > of the error return and that you need to handle it for other > syscalls. > My interpretation of the signal man page would be that either > you have a > signal handler with the flag and it restarts, or you don't and > you get > EINTR. But I can't find any signal handler that would be set in > those tests > that would have SA_RESTART, so it would be possible to see it in > userspace. > > I did see it in strace, so it seems to be restarting and not > ending up in > userspace, but I feel like I'm missing something to understand. > >>>>> + >>>>> +��� cur_brk = (uintptr_t)ret; > >>>>> ����� for (i = 0; i < 33; i++) { >>>>> ��������� switch (i % 3) { >>>>> @@ -31,11 +39,17 @@ void verify_brk(void) >>>>> ��������� break; >>>>> ��������� } >>>>> � -������� TST_EXP_PASS_SILENT(brk((void >>>>> *)new_brk), "brk()"); >>>>> -������� if (!TST_PASS) >>>>> +������� tst_syscall(__NR_brk, new_brk); >>>> In the original code, if brk((void *)new_brk), "brk()") fails, >>>> the test prints a TFAIL message and ends. This doesn't happen >>>> anymore. Just keep the TST_EXP_PASS_SILENT and only replace the >>>> brk(..) with tst_syscall(__NR_brk, ...). >>> According to the documentation : >>> >>> �> The 'TST_EXP_PASS()' can be used for calls that return -1 on >>> failure and 0 on >>> �> success. It will check for the return value and reports >>> failure >>> if the return >>> �> value is not equal to 0. The call also sets the 'TST_PASS' >>> variable to 1 if >>> �> the call succeeeded. >>> >>> As tst_syscall returns with the return value of the syscall, I >>> don't >>> believe it would work. (The only difference with the _SILENT >>> version >>> is the announcement or not of the PASS.) >>> I could create a wrapper to replace that but that seems >>> unnecessary >>> as I already check that it hasn't errored and the following test >>> checks that the result is correct. >> Indeed, it wouldn't work. Also, doing brk twice seems >> unnecessary. I >> think you could assign "cur_break = tst_syscall(__NR_brk, >> new_brk)" >> directly and just keep the check for cur_brk != new_brk. Would >> that >> work? > I believe it would indeed, I will make the changes. I tried to > stay close to > the original code which is why it is weirdly duplicated. Alternatively you could use either TESTPTR and check 'new_brk' against TST_RET_PTR, or TST_EXP_VAL_SILENT (also against the 'new_brk'). Or just compare 'new_brk' against 'cur_brk' as suggested by Tudor.

I did prepare a change by just checking 'new_brk' against 'cur_brk' but it might be more proper with what you suggest, I'll give it a look.

...

---

BR B.

Thanks for the comments ! T�o

...

>>>>> + >>>>> +������� ret = tst_syscall(__NR_brk, 0); >>>>> + >>>>> +������� if (ret < 0) >>>>> +���� > +����������� >>>>> tst_res(TFAIL, "Failed to get current break"); >>>>> ������������� return; >>>>> +������� } >>>>> � -������� cur_brk = (uintptr_t)sbrk(0); >>>>> +������� cur_brk = (uintptr_t)ret; >>>>> � ��������� if (cur_brk != new_brk) { >>>>> ������������� tst_res(TFAIL, >>>>> diff --git a/testcases/kernel/syscalls/brk/brk02.c >>>>> b/testcases/kernel/syscalls/brk/brk02.c >>>>> index 11e803cb4..19514febb 100644 >>>>> --- a/testcases/kernel/syscalls/brk/brk02.c >>>>> +++ b/testcases/kernel/syscalls/brk/brk02.c >>>>> @@ -14,24 +14,27 @@ >>>>> � #include <unistd.h> >>>>> � #include <sys/mman.h> >>>>> � #include "tst_test.h" >>>>> +#include "lapi/syscalls.h" >>>>> � � void brk_down_vmas(void) >>>>> � { >>>>> -��� void *brk_addr = sbrk(0); >>>>> +��� long ret = tst_syscall(__NR_brk, 0); >>>> I suggest keeping brk_addr here as well. No need to store the >>>> result into a long that you cast to void* afterwards. You can >>>> change the brk_addr's type to uintptr_t though if you want. >>> Same as above, will change depending on what we decide. >>>>> � -��� if (brk_addr == (void *) -1) >>>>> -������� tst_brk(TBROK, "sbrk() failed"); >>>>> +��� if (ret < 0) >>>>> +������� tst_brk(TBROK, "Failed to get current >>>>> break"); >>>> Same as above, tst_syscall(__NR_brk, 0) never returns below 0, >>>> so no need to check it. >>> Same as above, will change depending on discussion. >>> >>>> Good patch overall, only a few nits! Make sure to verify the LTP >>>> patches against checkpatch as well. >>> Thanks for the review Tudor ! >> Thanks, >> Tudor >> > Thanks, > T�o >>>>> + >>>>> +��� void *brk_addr = (void *) ret; >>>>> � ����� unsigned long page_size = getpagesize(); >>>>> ����� void *addr = brk_addr + page_size; >>>>> � -��� if (brk(addr)) { >>>>> +��� if (tst_syscall(__NR_brk, addr) < addr) { >>>>> ��������� tst_res(TFAIL | TERRNO, "Cannot >>>>> expand brk() by page size"); >>>>> ��������� return; >>>>> ����� } >>>>> � ����� addr += page_size; >>>>> -��� if (brk(addr)) { >>>>> +��� if (tst_syscall(__NR_brk, addr) < addr) { >>>>> ��������� tst_res(TFAIL | TERRNO, "Cannot >>>>> expand brk() by 2x >>>>> page size"); >>>>> ��������� return; >>>>> ����� } >>>>> @@ -42,12 +45,12 @@ void brk_down_vmas(void) >>>>> ����� } >>>>> � ����� addr += page_size; >>>>> -��� if (brk(addr)) { >>>>> +��� if (tst_syscall(__NR_brk, addr) < addr) { >>>>> ��������� tst_res(TFAIL | TERRNO, "Cannot >>>>> expand brk() after >>>>> mprotect"); >>>>> ��������� return; >>>>> ����� } >>>>> � -��� if (brk(brk_addr)) { >>>>> +��� if (tst_syscall(__NR_brk, brk_addr) != brk_addr) { >>>>> ��������� tst_res(TFAIL | TERRNO, "Cannot >>>>> restore brk() to >>>>> start address"); >>>>> ��������� return; >>>>> ����� } > _______________________________________________ > 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

Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

brk is disabled in purecap so in normal operation the test would be skipped by LTP. If that is not the case, guarantee the test will fail.

Add brk01 and brk02 to the morello_transitional_extended list.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

runtest/morello_transitional_extended | 2 ++ testcases/kernel/syscalls/brk/brk01.c | 13 +++++++++++++ 2 files changed, 15 insertions(+)

diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..8ddab7095 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,7 @@ #DESCRIPTION: Morello transitional extended ABI system calls +brk01 brk02

This should be: +brk01 brk01 +brk02 brk02

• epoll_create01 epoll_create01 epoll_create02 epoll_create02 epoll_create1_01 epoll_create1_01

diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index c16b46eaa..5157a5605 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c

Shouldn't a similar change be needed for brk02.c ?

@@ -9,7 +9,19 @@ #include <errno.h> #include "tst_test.h" +#include "lapi/syscalls.h" +#ifdef __CHERI_PURE_CAPABILITY__ +void verify_brk(void) +{

• /*

• * tst_syscall skips the test if the syscall returns -ENOSYS,
  

• * which is the expected behavior in purecap.
  

• */
  

• tst_syscall(__NR_brk, 0);
• tst_res(TFAIL, "brk should not be implemented in purecap");

+} +#else

nit: I would prefer to have a single verify_brk(void), and the ifdef __CHERI_PURE_CAPABILITY__ block should be inside the function. Up to you though.

Thanks, Tudor

void verify_brk(void) { uintptr_t cur_brk, new_brk; @@ -65,6 +77,7 @@ void verify_brk(void) tst_res(TPASS, "brk() works fine"); } +#endif /* __CHERI_PURE_CAPABILITY__ */ static struct tst_test test = { .test_all = verify_brk,

On 18-10-2022 13:55, Teo Couprie Diaz wrote:

On 18/10/2022 13:33, Tudor Cretu wrote:

...

Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

brk is disabled in purecap so in normal operation the test would be skipped by LTP. If that is not the case, guarantee the test will fail.

Add brk01 and brk02 to the morello_transitional_extended list.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

runtest/morello_transitional_extended |  2 ++   testcases/kernel/syscalls/brk/brk01.c | 13 +++++++++++++   2 files changed, 15 insertions(+)

diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..8ddab7095 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,7 @@   #DESCRIPTION: Morello transitional extended ABI system calls   +brk01 brk02

This should be: +brk01 brk01 +brk02 brk02

Right, my mistake.

...

...

epoll_create01 epoll_create01   epoll_create02 epoll_create02   epoll_create1_01 epoll_create1_01 diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index c16b46eaa..5157a5605 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c

Shouldn't a similar change be needed for brk02.c ?

It could, but I chose not do. My reasoning is that brk02 will SKIP in purecap if it's implemented correctly. If that's not the case, brk01 will FAIL, well, without fail. To me we don't gain more information in forcing a failure in brk02 like in brk01, they end up being the exact same tests in purecap while being completely different in compat : brk01 testing the basic availability and brk02 testing more complex scenarios.

What do you think ?

Sounds fine to me. If you want, you can mention why it's fine to keep brk02.c as it is in the commit message.

Thanks, Tudor

...

...

@@ -9,7 +9,19 @@   #include <errno.h>     #include "tst_test.h" +#include "lapi/syscalls.h"   +#ifdef __CHERI_PURE_CAPABILITY__ +void verify_brk(void) +{ +    /* +     * tst_syscall skips the test if the syscall returns -ENOSYS, +     * which is the expected behavior in purecap. +     */ +    tst_syscall(__NR_brk, 0); +    tst_res(TFAIL, "brk should not be implemented in purecap"); +} +#else

nit: I would prefer to have a single verify_brk(void), and the ifdef __CHERI_PURE_CAPABILITY__ block should be inside the function. Up to you though.

That's fair !

...

Thanks, Tudor

Thanks for the review, Téo

...

...

void verify_brk(void)   {       uintptr_t cur_brk, new_brk; @@ -65,6 +77,7 @@ void verify_brk(void)         tst_res(TPASS, "brk() works fine");   } +#endif /* __CHERI_PURE_CAPABILITY__ */     static struct tst_test test = {       .test_all = verify_brk,

On 19/10/2022 15:35, Beata Michalska wrote:

On Tue, Oct 18, 2022 at 03:23:54PM +0100, Tudor Cretu wrote:

...

On 18-10-2022 13:55, Teo Couprie Diaz wrote:

...

On 18/10/2022 13:33, Tudor Cretu wrote:

...

Hi Teo,

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

brk is disabled in purecap so in normal operation the test would be skipped by LTP. If that is not the case, guarantee the test will fail.

Add brk01 and brk02 to the morello_transitional_extended list.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

� runtest/morello_transitional_extended |� 2 ++ � testcases/kernel/syscalls/brk/brk01.c | 13 +++++++++++++ � 2 files changed, 15 insertions(+)

diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..8ddab7095 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,7 @@ � #DESCRIPTION: Morello transitional extended ABI system calls � +brk01 brk02

This should be: +brk01 brk01 +brk02 brk02

Right, my mistake.

...

...

� epoll_create01 epoll_create01 � epoll_create02 epoll_create02 � epoll_create1_01 epoll_create1_01 diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index c16b46eaa..5157a5605 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c

Shouldn't a similar change be needed for brk02.c ?

It could, but I chose not do. My reasoning is that brk02 will SKIP in purecap if it's implemented correctly. If that's not the case, brk01 will FAIL, well, without fail. To me we don't gain more information in forcing a failure in brk02 like in brk01, they end up being the exact same tests in purecap while being completely different in compat : brk01 testing the basic availability and brk02 testing more complex scenarios.

What do you think ?

Sounds fine to me. If you want, you can mention why it's fine to keep brk02.c as it is in the commit message.

Actually it would be very much appreciated if the justification for that would end up in the commit message.

I agree, I did add in preparation for the v2. However as I will call syscall() directly now, I might have to change the behavior a bit as brk02 won't be skipped automatically then.

---

BR B.

Thanks, Téo

...

Thanks, Tudor

...

...

...

@@ -9,7 +9,19 @@ � #include <errno.h> � � #include "tst_test.h" +#include "lapi/syscalls.h" � +#ifdef __CHERI_PURE_CAPABILITY__ +void verify_brk(void) +{ +��� /* +���� * tst_syscall skips the test if the syscall returns -ENOSYS, +���� * which is the expected behavior in purecap. +���� */ +��� tst_syscall(__NR_brk, 0); +��� tst_res(TFAIL, "brk should not be implemented in purecap"); +} +#else

nit: I would prefer to have a single verify_brk(void), and the ifdef __CHERI_PURE_CAPABILITY__ block should be inside the function. Up to you though.

That's fair !

...

Thanks, Tudor

Thanks for the review, T�o

...

...

� void verify_brk(void) � { ����� uintptr_t cur_brk, new_brk; @@ -65,6 +77,7 @@ void verify_brk(void) � ����� tst_res(TPASS, "brk() works fine"); � } +#endif /* __CHERI_PURE_CAPABILITY__ */ � � static struct tst_test test = { ����� .test_all = verify_brk,

---

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 18/10/2022 17:09, Tudor Cretu wrote:

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

brk is disabled in purecap so in normal operation the test would be skipped by LTP. If that is not the case, guarantee the test will fail.

Add brk01 and brk02 to the morello_transitional_extended list.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

runtest/morello_transitional_extended |  2 ++   testcases/kernel/syscalls/brk/brk01.c | 13 +++++++++++++   2 files changed, 15 insertions(+)

diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..8ddab7095 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,7 @@   #DESCRIPTION: Morello transitional extended ABI system calls   +brk01 brk02

epoll_create01 epoll_create01   epoll_create02 epoll_create02   epoll_create1_01 epoll_create1_01 diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index c16b46eaa..5157a5605 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,7 +9,19 @@   #include <errno.h>     #include "tst_test.h" +#include "lapi/syscalls.h"   +#ifdef __CHERI_PURE_CAPABILITY__ +void verify_brk(void) +{ +    /* +     * tst_syscall skips the test if the syscall returns -ENOSYS, +     * which is the expected behavior in purecap. +     */ +    tst_syscall(__NR_brk, 0); +    tst_res(TFAIL, "brk should not be implemented in purecap");

Maybe I am too back and forth on this topic, in which case I apologize! Just wanted to leave this here: an alternative would be to make the brk tests pass actively, something like:

TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap");

Again, up to you really.

I didn't really have any preference, but I just realised that tst_syscall's return is an int. It always worked while testing but it could very much not if the address is large enough. So I guess the best solution would be to use syscall directly anyway and bypass LTP's utility function, which would make it a PASS in purecap and not a SKIP.

Thanks, Tudor

Thanks, Téo

...

+} +#else   void verify_brk(void)   {       uintptr_t cur_brk, new_brk; @@ -65,6 +77,7 @@ void verify_brk(void)         tst_res(TPASS, "brk() works fine");   } +#endif /* __CHERI_PURE_CAPABILITY__ */     static struct tst_test test = {       .test_all = verify_brk,

On 19/10/2022 15:38, Beata Michalska wrote:

On Wed, Oct 19, 2022 at 12:45:27PM +0100, Teo Couprie Diaz wrote:

...

On 18/10/2022 17:09, Tudor Cretu wrote:

...

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

brk is disabled in purecap so in normal operation the test would be skipped by LTP. If that is not the case, guarantee the test will fail.

Add brk01 and brk02 to the morello_transitional_extended list.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

� runtest/morello_transitional_extended |� 2 ++ � testcases/kernel/syscalls/brk/brk01.c | 13 +++++++++++++ � 2 files changed, 15 insertions(+)

diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..8ddab7095 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,7 @@ � #DESCRIPTION: Morello transitional extended ABI system calls � +brk01 brk02

� epoll_create01 epoll_create01 � epoll_create02 epoll_create02 � epoll_create1_01 epoll_create1_01 diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index c16b46eaa..5157a5605 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,7 +9,19 @@ � #include <errno.h> � � #include "tst_test.h" +#include "lapi/syscalls.h" � +#ifdef __CHERI_PURE_CAPABILITY__ +void verify_brk(void) +{ +��� /* +���� * tst_syscall skips the test if the syscall returns -ENOSYS, +���� * which is the expected behavior in purecap. +���� */ +��� tst_syscall(__NR_brk, 0); +��� tst_res(TFAIL, "brk should not be implemented in purecap");

Maybe I am too back and forth on this topic, in which case I apologize! Just wanted to leave this here: an alternative would be to make the brk tests pass actively, something like:

� TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap");

Again, up to you really.

I didn't really have any preference, but I just realised that tst_syscall's return is an int. It always worked while testing but it could very much not if the address is large enough. So I guess the best solution would be to use syscall directly anyway and bypass LTP's utility function, which would make it a PASS in purecap and not a SKIP.

That sounds very much sensible. Aside: having tst_syscall using int for the syscall return value might be problematic in either cases as well - might be worth to actually modify that one ?

It does feel a bit weird. My understanding is that "by default" the return type of a syscall is long ? (As there doesn't seem to be a slot in the SYSCALL_DEFINE macro for a return type) Prompted by your comment I had a further look around and there are other examples where this leads to incorrect return types. This is the case for brk here, but also for clone3 for example (include/lapi/clone.h, line 30) where the return type is int where it should be long.

Anyways for this particular case it might be enough to simply use syscall directly without any wrappers, as suggested.

I'll do that for now, but maybe if we try to share the patch upstream it would be worth changing tst_syscall ? I don't really now !

---

BR B.

Thanks, Téo

...

...

Thanks, Tudor

Thanks, T�o

...

...

+} +#else � void verify_brk(void) � { ����� uintptr_t cur_brk, new_brk; @@ -65,6 +77,7 @@ void verify_brk(void) � ����� tst_res(TPASS, "brk() works fine"); � } +#endif /* __CHERI_PURE_CAPABILITY__ */ � � static struct tst_test test = { ����� .test_all = verify_brk,

---

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 19/10/2022 18:09, Beata Michalska wrote:

On Wed, Oct 19, 2022 at 04:19:20PM +0100, Teo Couprie Diaz wrote:

...

On 19/10/2022 15:38, Beata Michalska wrote:

...

On Wed, Oct 19, 2022 at 12:45:27PM +0100, Teo Couprie Diaz wrote:

...

On 18/10/2022 17:09, Tudor Cretu wrote:

...

On 18-10-2022 09:46, Teo Couprie Diaz wrote:

...

brk is disabled in purecap so in normal operation the test would be skipped by LTP. If that is not the case, guarantee the test will fail.

Add brk01 and brk02 to the morello_transitional_extended list.

Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com

� runtest/morello_transitional_extended |� 2 ++ � testcases/kernel/syscalls/brk/brk01.c | 13 +++++++++++++ � 2 files changed, 15 insertions(+)

diff --git a/runtest/morello_transitional_extended b/runtest/morello_transitional_extended index 067fe82da..8ddab7095 100644 --- a/runtest/morello_transitional_extended +++ b/runtest/morello_transitional_extended @@ -1,5 +1,7 @@ � #DESCRIPTION: Morello transitional extended ABI system calls � +brk01 brk02

� epoll_create01 epoll_create01 � epoll_create02 epoll_create02 � epoll_create1_01 epoll_create1_01 diff --git a/testcases/kernel/syscalls/brk/brk01.c b/testcases/kernel/syscalls/brk/brk01.c index c16b46eaa..5157a5605 100644 --- a/testcases/kernel/syscalls/brk/brk01.c +++ b/testcases/kernel/syscalls/brk/brk01.c @@ -9,7 +9,19 @@ � #include <errno.h> � � #include "tst_test.h" +#include "lapi/syscalls.h" � +#ifdef __CHERI_PURE_CAPABILITY__ +void verify_brk(void) +{ +��� /* +���� * tst_syscall skips the test if the syscall returns -ENOSYS, +���� * which is the expected behavior in purecap. +���� */ +��� tst_syscall(__NR_brk, 0); +��� tst_res(TFAIL, "brk should not be implemented in purecap");

Maybe I am too back and forth on this topic, in which case I apologize! Just wanted to leave this here: an alternative would be to make the brk tests pass actively, something like:

� TST_EXP_FAIL(syscall(__NR_brk, 0), ENOSYS, "brk is not implemented in purecap");

Again, up to you really.

I didn't really have any preference, but I just realised that tst_syscall's return is an int. It always worked while testing but it could very much not if the address is large enough. So I guess the best solution would be to use syscall directly anyway and bypass LTP's utility function, which would make it a PASS in purecap and not a SKIP.

That sounds very much sensible. Aside: having tst_syscall using int for the syscall return value might be problematic in either cases as well - might be worth to actually modify that one ?

It does feel a bit weird. My understanding is that "by default" the return type of a syscall is long ? (As there doesn't seem to be a slot in the SYSCALL_DEFINE macro for a return type) Prompted by your comment I had a further look around and there are other examples where this leads to incorrect return types. This is the case for brk here, but also for clone3 for example (include/lapi/clone.h, line 30) where the return type is int where it should be long.

Actually clone3 returns pid_t type (int), so it should be just fine there, but there are other ones (like all the read/write family with ssize_t) that might be affected - I somehow feel like I am missing smth here ...

Indeed it does, weirdly the man page (on my machine) for clone has `long clone3(....)` for the prototype. But I agree, it feels that we might be missing something. Maybe it's just a long-standing issue that hasn't really been hit ?

SYSCALL_DEFINE will be evaluated to SYSCALL_DEFINEx(...) where you can see how the syscall handler prototypes are being defined (@see arch/arm64/include/asm/syscall_wrapper.h) [...]

Which is what I did to reach this conclusion indeed :)

[...] and yes, the type for the return value is by default long (type that fits into the registers), though for PCuABI we had to make it as uintcap_t for certain set of syscalls where we do expect to be handed over a pointer (for Morello we have capability registers). You can have a look at 'man syscall' which should give you a table showing which registers are being used.

That makes a lot of sense, that's what I supposed was happening but thanks for confirming ! The syscall man page does have quite a lot of interesting information, thanks for pointing it out.

...

...

Anyways for this particular case it might be enough to simply use syscall directly without any wrappers, as suggested.

I'll do that for now, but maybe if we try to share the patch upstream it would be worth changing tst_syscall ? I don't really now !

I am kinda puzzled at this point to be honest - need to get a grip on that one. Let's make one step at a time.

Of course, no worries on that front.

---

BR B.

Thanks, Téo

...

...

---

BR B.

Thanks, T�o

...

...

...

Thanks, Tudor

Thanks, T�o

...

...

+} +#else � void verify_brk(void) � { ����� uintptr_t cur_brk, new_brk; @@ -65,6 +77,7 @@ void verify_brk(void) � ����� tst_res(TPASS, "brk() works fine"); � } +#endif /* __CHERI_PURE_CAPABILITY__ */ � � static struct tst_test test = { ����� .test_all = verify_brk,

---

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