Commit 41348659371e ("setpgid02: Temporarily disable EPERM test case") disabled a test case that failed in CI due to its configuration.
This temporary measure is now suprseded by the fixes upstream. Revert this commit and cherry-pick the relevant upstream commits.
These commits will be in the upcoming (in a few weeks) LTP release, so it might not be needed to cherry-pick them now.
Teo Couprie Diaz (3): Revert "setpgid02: Temporarily disable EPERM test case" setpgid02: Use pid_max as PGID for EPERM setpgid03: Add test for PGID in different session
testcases/kernel/syscalls/setpgid/setpgid02.c | 19 +++++++------------ testcases/kernel/syscalls/setpgid/setpgid03.c | 4 ++++ 2 files changed, 11 insertions(+), 12 deletions(-)
This reverts commit 41348659371e02602cf79e227af4782340a42512. --- testcases/kernel/syscalls/setpgid/setpgid02.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c b/testcases/kernel/syscalls/setpgid/setpgid02.c index 4dcb7613d..4b63afee8 100644 --- a/testcases/kernel/syscalls/setpgid/setpgid02.c +++ b/testcases/kernel/syscalls/setpgid/setpgid02.c @@ -31,12 +31,7 @@ static struct tcase { } tcases[] = { {&pid, &negative_pid, EINVAL}, {&ppid, &pgid, ESRCH}, - /* - * FIXME: This test case makes an assumption that is invalid in our CI. - * Skip it for now as the error case is covered in setpgid03 anyway. - * Should be restored when the fix is decided with upstream. - */ -/* {&pid, &init_pgid, EPERM} */ + {&pid, &init_pgid, EPERM} };
static void setup(void)
In some simple systems (like Busybox), the login shell might be run as init (PID 1). This leads to a case where LTP is run in the same session as init, thus setpgid is allowed to the PGID of init which results in a test fail. Indeed, the test retrieves the PGID of init to try and generate EPERM.
Instead get the PGID we use to generate EPERM from the kernel pid_max. It should not be used by any process, guaranteeing an invalid PGID and generating an EPERM error.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com Reviewed-by: Li Wang liwang@redhat.com Reviewed-by: Cyril Hrubis chrubis@suse.cz --- testcases/kernel/syscalls/setpgid/setpgid02.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c b/testcases/kernel/syscalls/setpgid/setpgid02.c index 4b63afee8..b380d7df4 100644 --- a/testcases/kernel/syscalls/setpgid/setpgid02.c +++ b/testcases/kernel/syscalls/setpgid/setpgid02.c @@ -13,15 +13,15 @@ * - EINVAL when given pgid is less than 0. * - ESRCH when pid is not the calling process and not a child of * the calling process. - * - EPERM when an attempt was made to move a process into a process - * group in a different session. + * - EPERM when an attempt was made to move a process into a nonexisting + * process group. */
#include <errno.h> #include <unistd.h> #include "tst_test.h"
-static pid_t pgid, pid, ppid, init_pgid; +static pid_t pgid, pid, ppid, inval_pgid; static pid_t negative_pid = -1;
static struct tcase { @@ -31,7 +31,7 @@ static struct tcase { } tcases[] = { {&pid, &negative_pid, EINVAL}, {&ppid, &pgid, ESRCH}, - {&pid, &init_pgid, EPERM} + {&pid, &inval_pgid, EPERM} };
static void setup(void) @@ -41,10 +41,10 @@ static void setup(void) pgid = getpgrp();
/* - * Getting pgid of init/systemd process to use it as a - * process group from a different session for EPERM test + * pid_max would not be in use by another process and guarantees that + * it corresponds to an invalid PGID, generating EPERM. */ - init_pgid = SAFE_GETPGID(1); + SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%d\n", &inval_pgid); }
static void run(unsigned int n)
The current test in setpgid03 generates EPERM because the child is a session leader, as it has called setsid(). EPERM can also happen by trying to change to a PGID in another session. This was previously done in setpgid02, but it could fail on some systems.
setpgid03 provides a guaranteed way to generate this error by forking and setsid() in the child, so add a test for it here.
Update the description to reflect this understanding.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com Reviewed-by: Li Wang liwang@redhat.com Reviewed-by: Cyril Hrubis chrubis@suse.cz --- testcases/kernel/syscalls/setpgid/setpgid03.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/testcases/kernel/syscalls/setpgid/setpgid03.c b/testcases/kernel/syscalls/setpgid/setpgid03.c index 79ca23e08..b747dacaf 100644 --- a/testcases/kernel/syscalls/setpgid/setpgid03.c +++ b/testcases/kernel/syscalls/setpgid/setpgid03.c @@ -11,6 +11,8 @@ * * Tests setpgid() erorrs: * + * - EPERM The process specified by pid must not be a session leader. + * * - EPERM The calling process, process specified by pid and the target * process group must be in the same session. * @@ -43,6 +45,8 @@ static void run(void) TST_CHECKPOINT_WAIT(0);
TST_EXP_FAIL(setpgid(child_pid, getppid()), EPERM); + /* Child did setsid(), so its PGID is set to its PID. */ + TST_EXP_FAIL(setpgid(0, child_pid), EPERM);
TST_CHECKPOINT_WAKE(0);
On 26/04/2023 14:30, Teo Couprie Diaz wrote:
Commit 41348659371e ("setpgid02: Temporarily disable EPERM test case") disabled a test case that failed in CI due to its configuration.
This temporary measure is now suprseded by the fixes upstream. Revert this commit and cherry-pick the relevant upstream commits.
These commits will be in the upcoming (in a few weeks) LTP release, so it might not be needed to cherry-pick them now.
Teo Couprie Diaz (3): Revert "setpgid02: Temporarily disable EPERM test case" setpgid02: Use pid_max as PGID for EPERM setpgid03: Add test for PGID in different session
testcases/kernel/syscalls/setpgid/setpgid02.c | 19 +++++++------------ testcases/kernel/syscalls/setpgid/setpgid03.c | 4 ++++ 2 files changed, 11 insertions(+), 12 deletions(-)
I think it makes sense to get this series in. It will avoid a conflict next time we merge upstream LTP.
Kevin
On Wed, Apr 26, 2023 at 05:18:59PM +0200, Kevin Brodsky wrote:
On 26/04/2023 14:30, Teo Couprie Diaz wrote:
Commit 41348659371e ("setpgid02: Temporarily disable EPERM test case") disabled a test case that failed in CI due to its configuration.
This temporary measure is now suprseded by the fixes upstream. Revert this commit and cherry-pick the relevant upstream commits.
These commits will be in the upcoming (in a few weeks) LTP release, so it might not be needed to cherry-pick them now.
Teo Couprie Diaz (3): Revert "setpgid02: Temporarily disable EPERM test case" setpgid02: Use pid_max as PGID for EPERM setpgid03: Add test for PGID in different session
testcases/kernel/syscalls/setpgid/setpgid02.c | 19 +++++++------------ testcases/kernel/syscalls/setpgid/setpgid03.c | 4 ++++ 2 files changed, 11 insertions(+), 12 deletions(-)
I think it makes sense to get this series in. It will avoid a conflict next time we merge upstream LTP.
Indeed, though it would be good to: - potentially align the revert commit to how it has been handled so far (e.g.: https://git.morello-project.org/morello/morello-linux-ltp/-/commit/ca598855 ) - for the cherry-picks it would be good to mark them as such: (e.g. git cherry-pick -xs .... )
I can amend the patches if you are ok with it.
--- BR B.
Kevin _______________________________________________ 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 Beata,
On 02/05/2023 15:01, Beata Michalska wrote:
On Wed, Apr 26, 2023 at 05:18:59PM +0200, Kevin Brodsky wrote:
On 26/04/2023 14:30, Teo Couprie Diaz wrote:
Commit 41348659371e ("setpgid02: Temporarily disable EPERM test case") disabled a test case that failed in CI due to its configuration.
This temporary measure is now suprseded by the fixes upstream. Revert this commit and cherry-pick the relevant upstream commits.
These commits will be in the upcoming (in a few weeks) LTP release, so it might not be needed to cherry-pick them now.
Teo Couprie Diaz (3): Revert "setpgid02: Temporarily disable EPERM test case" setpgid02: Use pid_max as PGID for EPERM setpgid03: Add test for PGID in different session
testcases/kernel/syscalls/setpgid/setpgid02.c | 19 +++++++------------ testcases/kernel/syscalls/setpgid/setpgid03.c | 4 ++++ 2 files changed, 11 insertions(+), 12 deletions(-)
I think it makes sense to get this series in. It will avoid a conflict next time we merge upstream LTP.
Indeed, though it would be good to:
- potentially align the revert commit to how it has been handled so far (e.g.: https://git.morello-project.org/morello/morello-linux-ltp/-/commit/ca598855 )
- for the cherry-picks it would be good to mark them as such: (e.g. git cherry-pick -xs .... )
Thanks for the comments, I agree this would be much better. Sorry for sending it out without looking better into how it should be done!
I can amend the patches if you are ok with it.
I'm ok for you to do it if you prefer, thanks in advance.
Best regards Téo
BR B.
Kevin _______________________________________________ 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 Tue, May 02, 2023 at 03:43:40PM +0100, Teo Couprie Diaz wrote:
Hi Beata,
On 02/05/2023 15:01, Beata Michalska wrote:
On Wed, Apr 26, 2023 at 05:18:59PM +0200, Kevin Brodsky wrote:
On 26/04/2023 14:30, Teo Couprie Diaz wrote:
Commit 41348659371e ("setpgid02: Temporarily disable EPERM test case") disabled a test case that failed in CI due to its configuration.
This temporary measure is now suprseded by the fixes upstream. Revert this commit and cherry-pick the relevant upstream commits.
These commits will be in the upcoming (in a few weeks) LTP release, so it might not be needed to cherry-pick them now.
Teo Couprie Diaz (3): Revert "setpgid02: Temporarily disable EPERM test case" setpgid02: Use pid_max as PGID for EPERM setpgid03: Add test for PGID in different session
testcases/kernel/syscalls/setpgid/setpgid02.c | 19 +++++++------------ testcases/kernel/syscalls/setpgid/setpgid03.c | 4 ++++ 2 files changed, 11 insertions(+), 12 deletions(-)
I think it makes sense to get this series in. It will avoid a conflict next time we merge upstream LTP.
Indeed, though it would be good to:
- potentially align the revert commit to how it has been handled so far (e.g.: https://git.morello-project.org/morello/morello-linux-ltp/-/commit/ca598855 )
- for the cherry-picks it would be good to mark them as such: (e.g. git cherry-pick -xs .... )
Thanks for the comments, I agree this would be much better. Sorry for sending it out without looking better into how it should be done!
I can amend the patches if you are ok with it.
I'm ok for you to do it if you prefer, thanks in advance.
Landed on next. (I have added your sing-off for revert commit - shout if you want it to be removed)
--- BR B.
Best regards Téo
BR B.
Kevin _______________________________________________ linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
linux-morello-ltp mailing list -- linux-morello-ltp@op-lists.linaro.org To unsubscribe send an email to linux-morello-ltp-leave@op-lists.linaro.org
linux-morello-ltp@op-lists.linaro.org
-
Beata Michalska -
Kevin Brodsky -
Teo Couprie Diaz