[LTP] [PATCH] setpgid02: Use pid_max as PGID for EPERM

[LTP] [PATCH] setpgid02: Use pid_max as PGID for EPERM

[Teo Couprie Diaz]

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 a different session
and generating an EPERM error.

Signed-off-by: Teo Couprie Diaz <teo.coupriediaz@arm.com>
---
See discussion on the origin of this patch (end of thread):
https://lists.linux.it/pipermail/ltp/2023-April/033505.html

Apologies for the time it took to send this.

CI Build:
https://github.com/Teo-CD/ltp/actions/runs/4732620255

 testcases/kernel/syscalls/setpgid/setpgid02.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c b/testcases/kernel/syscalls/setpgid/setpgid02.c
index 4b63afee8..5341774e1 100644
--- a/testcases/kernel/syscalls/setpgid/setpgid02.c
+++ b/testcases/kernel/syscalls/setpgid/setpgid02.c
@@ -21,7 +21,7 @@
 #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 "another session", 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)
-- 
2.34.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] setpgid02: Use pid_max as PGID for EPERM

[Li Wang]

Reviewed-by: Li Wang <liwang@redhat.com>


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] setpgid02: Use pid_max as PGID for EPERM

[Cyril Hrubis]

Hi!
> 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 a different session
> and generating an EPERM error.

So I suppose that we hit slightly different condition in the kernel:

        if (pgid != pid) {
                struct task_struct *g;

                pgrp = find_vpid(pgid);
                g = pid_task(pgrp, PIDTYPE_PGID);
                if (!g || task_session(g) != task_session(group_leader))
                        goto out;
        }

Previously we were supposed to hit the task_session(g) !=
task_session(group_leader) now we hit !g.

Also I guess that the only way to hit the second part of the condition
is to actually open and initialize a pty so that we have a process
outside of the current session.

The patch itself looks okay, but we should at least update the
documentation comment such as:

diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c b/testcases/kernel/syscalls/setpgid/setpgid02.c
index 4b63afee8..68b663633 100644
--- a/testcases/kernel/syscalls/setpgid/setpgid02.c
+++ b/testcases/kernel/syscalls/setpgid/setpgid02.c
@@ -13,8 +13,8 @@
  * - 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.
  */

And ideally write a test for the second case as well.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] setpgid02: Use pid_max as PGID for EPERM

[Li Wang]

On Wed, Apr 19, 2023 at 6:59 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > 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 a different session
> > and generating an EPERM error.
>
> So I suppose that we hit slightly different condition in the kernel:
>
>         if (pgid != pid) {
>                 struct task_struct *g;
>
>                 pgrp = find_vpid(pgid);
>                 g = pid_task(pgrp, PIDTYPE_PGID);
>                 if (!g || task_session(g) != task_session(group_leader))
>                         goto out;
>         }
>
> Previously we were supposed to hit the task_session(g) !=
> task_session(group_leader) now we hit !g.
>

Ah, yes, thanks for pointing out the difference from the kernel layer.



>
> Also I guess that the only way to hit the second part of the condition
> is to actually open and initialize a pty so that we have a process
> outside of the current session.
>

+1, this sounds correct way to reach that branch.
We can add one more item in the tcase struct to cover it.



>
> The patch itself looks okay, but we should at least update the
> documentation comment such as:
>
> diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c
> b/testcases/kernel/syscalls/setpgid/setpgid02.c
> index 4b63afee8..68b663633 100644
> --- a/testcases/kernel/syscalls/setpgid/setpgid02.c
> +++ b/testcases/kernel/syscalls/setpgid/setpgid02.c
> @@ -13,8 +13,8 @@
>   * - 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.
>   */
>
> And ideally write a test for the second case as well.
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
>
>

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] setpgid02: Use pid_max as PGID for EPERM

[Teo Couprie Diaz]

Hi Cyril, Li, thanks for the review !

On 19/04/2023 12:11, Li Wang wrote:
>
>
> On Wed, Apr 19, 2023 at 6:59 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
>     Hi!
>     > 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 a different
>     session
>     > and generating an EPERM error.
>
>     So I suppose that we hit slightly different condition in the kernel:
>
>             if (pgid != pid) {
>                     struct task_struct *g;
>
>                     pgrp = find_vpid(pgid);
>                     g = pid_task(pgrp, PIDTYPE_PGID);
>                     if (!g || task_session(g) !=
>     task_session(group_leader))
>                             goto out;
>             }
>
>     Previously we were supposed to hit the task_session(g) !=
>     task_session(group_leader) now we hit !g.
>
>
> Ah, yes, thanks for pointing out the difference from the kernel layer.
>
>
>     Also I guess that the only way to hit the second part of the condition
>     is to actually open and initialize a pty so that we have a process
>     outside of the current session.
>
>
> +1, this sounds correct way to reach that branch.
> We can add one more item in the tcase struct to cover it.
The mechanism is indeed different. My first approach to this patch was 
to fork and setsid() the child, which
implied an EPERM due to the session difference.
However, when discussing this approach on the mailing list (see 
https://lists.linux.it/pipermail/ltp/2023-April/033505.html )
it was brought to my attention that setpgid03 is in fact doing exactly 
that already.

Knowing that, I didn't feel it would be worthwhile to add such a case in 
setpgid02.

However, I spent more time looking at the code on the kernel side 
prompted by your remark and I think
that setpgid03 is going through another path:

     if (same_thread_group(p->real_parent, group_leader)) {
         err = -EPERM;
         if (task_session(p) != task_session(group_leader))
             goto out;

So it might indeed make sense to add another case in setpgid02.

Would initializing a pty be necessary though ? Could it be simply 
achieved by spawning a child that
setsid() itself, and try to setpgid the parent to the child PGID ? 
(Rather than setpgid the child as in setpgid03)

Maybe it would make sense to add that case to setpgid03 rather than 
setpgid02, as setpgid03 already has
the necessary scaffolding ?

>
>
>     The patch itself looks okay, but we should at least update the
>     documentation comment such as:
>
>     diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c
>     b/testcases/kernel/syscalls/setpgid/setpgid02.c
>     index 4b63afee8..68b663633 100644
>     --- a/testcases/kernel/syscalls/setpgid/setpgid02.c
>     +++ b/testcases/kernel/syscalls/setpgid/setpgid02.c
>     @@ -13,8 +13,8 @@
>       * - 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.
>       */
>
Thanks, I missed that, will update in v2.

>     And ideally write a test for the second case as well.
>
Happy to do so, will wait for your thoughts on my responses above before 
sending a v2.
>
>
>     -- 
>     Cyril Hrubis
>     chrubis@suse.cz
>
>     -- 
>     Mailing list info: https://lists.linux.it/listinfo/ltp
>
>
>
> -- 
> Regards,
> Li Wang
Thanks both !
Best regards,
Téo

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] setpgid02: Use pid_max as PGID for EPERM

[Cyril Hrubis]

Hi!
> The mechanism is indeed different. My first approach to this patch was 
> to fork and setsid() the child, which
> implied an EPERM due to the session difference.
> However, when discussing this approach on the mailing list (see 
> https://lists.linux.it/pipermail/ltp/2023-April/033505.html )
> it was brought to my attention that setpgid03 is in fact doing exactly 
> that already.
> 
> Knowing that, I didn't feel it would be worthwhile to add such a case in 
> setpgid02.
> 
> However, I spent more time looking at the code on the kernel side 
> prompted by your remark and I think
> that setpgid03 is going through another path:
> 
>      if (same_thread_group(p->real_parent, group_leader)) {
>          err = -EPERM;
>          if (task_session(p) != task_session(group_leader))
>              goto out;
> 
> So it might indeed make sense to add another case in setpgid02.
> 
> Would initializing a pty be necessary though ? Could it be simply 
> achieved by spawning a child that
> setsid() itself, and try to setpgid the parent to the child PGID ? 
> (Rather than setpgid the child as in setpgid03)

Right, no need for a tty, we can just change the session.

> Maybe it would make sense to add that case to setpgid03 rather than 
> setpgid02, as setpgid03 already has
> the necessary scaffolding ?

That would be the best, we can simply add TST_EXP_FAIL() to the
do_child() in setpgid03. However please make sure to update the test
descriptions in both tests.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp