| Message ID | 20230418130944.181716-1-teo.coupriediaz@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | setpgid02: Use pid_max as PGID for EPERM | expand |
Reviewed-by: Li Wang <liwang@redhat.com>
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.
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 > >
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
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.
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)
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(-)