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

[Teo Couprie Diaz <teo.coupriediaz@arm.com>]

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