| Message ID | 20230331000747.2294390-1-edliaw@google.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2] setpgid02: use 1 instead of getpgid(1) | expand |
Hi Edward, > On Android, init does not setpgid, so getpgid(1) returns 0 and the third > case of setting pgid to a different session's process group does not > behave as expected: setpgid treats 0 as setting the pgid to the pid. > Instead, replace SAFE_GETPGID(1) with the expected value of 1. Seems to be safe. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
Hi Edward, On Friday, March 31, 2023 5:37:47 AM IST Edward Liaw via ltp wrote: > On Android, init does not setpgid, so getpgid(1) returns 0 and the third > case of setting pgid to a different session's process group does not > behave as expected: setpgid treats 0 as setting the pgid to the pid. > > Instead, replace SAFE_GETPGID(1) with the expected value of 1. > > Signed-off-by: Edward Liaw <edliaw@google.com> > --- > testcases/kernel/syscalls/setpgid/setpgid02.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c b/testcases/kernel/syscalls/setpgid/setpgid02.c > index 4b63afee8..bf7b3176b 100644 > --- a/testcases/kernel/syscalls/setpgid/setpgid02.c > +++ b/testcases/kernel/syscalls/setpgid/setpgid02.c > @@ -44,7 +44,7 @@ static void setup(void) > * Getting pgid of init/systemd process to use it as a > * process group from a different session for EPERM test > */ > - init_pgid = SAFE_GETPGID(1); > + init_pgid = 1; > } > > static void run(unsigned int n) > This looks fine. Reviewed-by: Avinesh Kumar <akumar@suse.de> Regards, Avinesh
Hi Edward, LTP folks, On 31/03/2023 01:07, Edward Liaw via ltp wrote: > On Android, init does not setpgid, so getpgid(1) returns 0 and the third > case of setting pgid to a different session's process group does not > behave as expected: setpgid treats 0 as setting the pgid to the pid. > > Instead, replace SAFE_GETPGID(1) with the expected value of 1. > > Signed-off-by: Edward Liaw <edliaw@google.com> > --- > testcases/kernel/syscalls/setpgid/setpgid02.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c b/testcases/kernel/syscalls/setpgid/setpgid02.c > index 4b63afee8..bf7b3176b 100644 > --- a/testcases/kernel/syscalls/setpgid/setpgid02.c > +++ b/testcases/kernel/syscalls/setpgid/setpgid02.c > @@ -44,7 +44,7 @@ static void setup(void) > * Getting pgid of init/systemd process to use it as a > * process group from a different session for EPERM test > */ > - init_pgid = SAFE_GETPGID(1); > + init_pgid = 1; > } > > static void run(unsigned int n) The change looks good and makes sense: Reviewed-by: Teo Couprie Diaz <teo.coupriediaz@arm.com> However, I have encountered an issue on the same check of this test, unrelated to Edward's issue. Indeed, on systems that run the shell as PID 1 (for example a basic busybox rootfs) the EPERM check wouldn't work. This is because LTP would run within the same session ID as init, which would allow the test to change the PGID and not trigger the EPERM. I am working on a patch and wanted to get some input. My current idea would be to fork a child that would create a new session and try to setpgid() the child. This would also allow to use the main process' PGID, as it would be in another session from the child anyway. This would remove the need to getpgid() init, which hopefully should fix your issue on Android as well. However, this adds a lot more complexity in the test: needing to fork and synchronize with the child as the main process needs to wait for the child to change its session ID, otherwise the test would fail. Do you think this idea makes sense ? I would send it for review once I ironed out the patch. Another solution would be for LTP to change its session ID by default, which would prevent the need for a change to setpgid02 on top of Edward's. However, I don't fully understand the possible consequences of having LTP change its SID for all tests. Happy to discuss or send an RFC patch. Thanks very much in advance, Téo-CD
On Fri, Apr 7, 2023 at 3:18 AM Teo Couprie Diaz <teo.coupriediaz@arm.com> wrote: > However, I have encountered an issue on the same check of this test, > unrelated to Edward's issue. > > Indeed, on systems that run the shell as PID 1 (for example a basic > busybox rootfs) the EPERM check wouldn't work. > This is because LTP would run within the same session ID as init, which > would allow the test to change the PGID and not trigger the EPERM. > > I am working on a patch and wanted to get some input. My current idea > would be to fork a child that would create a new session and try to > setpgid() the child. > This would also allow to use the main process' PGID, as it would be in > another session from the child anyway. This would remove the need to > getpgid() init, which hopefully should fix your issue on Android as well. > That makes sense to me, but it seems to me that setpgid03 is already testing it that way. > However, this adds a lot more complexity in the test: needing to fork > and synchronize with the child as the main process needs to wait for the > child to change its session ID, otherwise the test would fail. > > Do you think this idea makes sense ? I would send it for review once I > ironed out the patch. > Another solution would be for LTP to change its session ID by default, > which would prevent the need for a change to setpgid02 on top of Edward's. > However, I don't fully understand the possible consequences of having > LTP change its SID for all tests. > Alternatively, maybe it could be reverted to using the hardcoded 99999 as an invalid PGID as it was before Avinesh's patch or the test case removed because it is handled in setpgid03? Thanks, Edward
On 11/04/2023 00:51, Edward Liaw wrote: > On Fri, Apr 7, 2023 at 3:18 AM Teo Couprie Diaz <teo.coupriediaz@arm.com> wrote: >> However, I have encountered an issue on the same check of this test, >> unrelated to Edward's issue. >> >> Indeed, on systems that run the shell as PID 1 (for example a basic >> busybox rootfs) the EPERM check wouldn't work. >> This is because LTP would run within the same session ID as init, which >> would allow the test to change the PGID and not trigger the EPERM. >> >> I am working on a patch and wanted to get some input. My current idea >> would be to fork a child that would create a new session and try to >> setpgid() the child. >> This would also allow to use the main process' PGID, as it would be in >> another session from the child anyway. This would remove the need to >> getpgid() init, which hopefully should fix your issue on Android as well. >> > That makes sense to me, but it seems to me that setpgid03 is already > testing it that way. Ah, yes indeed it is testing it exactly like that. > >> However, this adds a lot more complexity in the test: needing to fork >> and synchronize with the child as the main process needs to wait for the >> child to change its session ID, otherwise the test would fail. >> >> Do you think this idea makes sense ? I would send it for review once I >> ironed out the patch. >> Another solution would be for LTP to change its session ID by default, >> which would prevent the need for a change to setpgid02 on top of Edward's. >> However, I don't fully understand the possible consequences of having >> LTP change its SID for all tests. >> > Alternatively, maybe it could be reverted to using the hardcoded 99999 > as an invalid PGID as it was before Avinesh's patch or the test case > removed because it is handled in setpgid03? I feel that it would make sense to remove the test case as it's tested as is in setpgid03. Even the comments for the EPERM cases are identical in meaning. If it is to be kept, I think it could be better to use the kernel pid_max rather than an hardcoded value (for example 99999 would be possible on my machine), but I agree it would be fine. Adding Petr Vorel to CCs as he reviewed Avinesh's patch. > > Thanks, > Edward Thanks for coming back to me, Best regards Téo
Hi all, [ Cc Avinesh, who also posted review ] > On 11/04/2023 00:51, Edward Liaw wrote: > > On Fri, Apr 7, 2023 at 3:18 AM Teo Couprie Diaz <teo.coupriediaz@arm.com> wrote: > > > However, I have encountered an issue on the same check of this test, > > > unrelated to Edward's issue. > > > Indeed, on systems that run the shell as PID 1 (for example a basic > > > busybox rootfs) the EPERM check wouldn't work. > > > This is because LTP would run within the same session ID as init, which > > > would allow the test to change the PGID and not trigger the EPERM. > > > I am working on a patch and wanted to get some input. My current idea > > > would be to fork a child that would create a new session and try to > > > setpgid() the child. > > > This would also allow to use the main process' PGID, as it would be in > > > another session from the child anyway. This would remove the need to > > > getpgid() init, which hopefully should fix your issue on Android as well. > > That makes sense to me, but it seems to me that setpgid03 is already > > testing it that way. > Ah, yes indeed it is testing it exactly like that. Good catch! > > > However, this adds a lot more complexity in the test: needing to fork > > > and synchronize with the child as the main process needs to wait for the > > > child to change its session ID, otherwise the test would fail. > > > Do you think this idea makes sense ? I would send it for review once I > > > ironed out the patch. > > > Another solution would be for LTP to change its session ID by default, > > > which would prevent the need for a change to setpgid02 on top of Edward's. > > > However, I don't fully understand the possible consequences of having > > > LTP change its SID for all tests. > > Alternatively, maybe it could be reverted to using the hardcoded 99999 > > as an invalid PGID as it was before Avinesh's patch or the test case > > removed because it is handled in setpgid03? > I feel that it would make sense to remove the test case as it's tested as is > in setpgid03. Even the comments for the EPERM cases are identical in > meaning. I don't want to add an ultimate answer (not sure myself), but IMHO these setpgid03.c and setpgid02.c aren't the same, because setpgid03.c calls: 1) the fork() you mentioned 2) setsid() (via SAFE_SETSID()) Therefore the EPERM meaning is the same, IMHO the code path in kernel and libc is not the same. > If it is to be kept, I think it could be better to use the kernel pid_max > rather than > an hardcoded value (for example 99999 would be possible on my machine), but > I agree it would be fine. Based to f2797fa44 commit message and my memory I guess Avinesh used PID 1 as that's 100% sure it's different from whatever process group could LTP test have. But IMHO that's not necessary, because PGID of both setpgid02 processes is always the same as PID: $ ps xao user,pid,ppid,pgid,sid,comm | grep -e ^USER -e setpgid02 USER PID PPID PGID SID COMMAND pevik 1822063 1820900 1822063 1820900 setpgid02 pevik 1822064 1822063 1822064 1820900 setpgid02 Therefore any PID would work => sure, scanning /proc/sys/kernel/pid_max LGTM: SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%lu", &pid_max); > Adding Petr Vorel to CCs as he reviewed Avinesh's patch. Thanks! I already posted my review, but missed following discussion. Kind regards, Petr > > Thanks, > > Edward > Thanks for coming back to me, > Best regards > Téo
Hi Petr, On 12/04/2023 00:05, Petr Vorel wrote: > Hi all, > > [ Cc Avinesh, who also posted review ] > >> On 11/04/2023 00:51, Edward Liaw wrote: >>> On Fri, Apr 7, 2023 at 3:18 AM Teo Couprie Diaz <teo.coupriediaz@arm.com> wrote: >>>> However, I have encountered an issue on the same check of this test, >>>> unrelated to Edward's issue. >>>> Indeed, on systems that run the shell as PID 1 (for example a basic >>>> busybox rootfs) the EPERM check wouldn't work. >>>> This is because LTP would run within the same session ID as init, which >>>> would allow the test to change the PGID and not trigger the EPERM. >>>> I am working on a patch and wanted to get some input. My current idea >>>> would be to fork a child that would create a new session and try to >>>> setpgid() the child. >>>> This would also allow to use the main process' PGID, as it would be in >>>> another session from the child anyway. This would remove the need to >>>> getpgid() init, which hopefully should fix your issue on Android as well. >>> That makes sense to me, but it seems to me that setpgid03 is already >>> testing it that way. >> Ah, yes indeed it is testing it exactly like that. > Good catch! > >>>> However, this adds a lot more complexity in the test: needing to fork >>>> and synchronize with the child as the main process needs to wait for the >>>> child to change its session ID, otherwise the test would fail. >>>> Do you think this idea makes sense ? I would send it for review once I >>>> ironed out the patch. >>>> Another solution would be for LTP to change its session ID by default, >>>> which would prevent the need for a change to setpgid02 on top of Edward's. >>>> However, I don't fully understand the possible consequences of having >>>> LTP change its SID for all tests. >>> Alternatively, maybe it could be reverted to using the hardcoded 99999 >>> as an invalid PGID as it was before Avinesh's patch or the test case >>> removed because it is handled in setpgid03? >> I feel that it would make sense to remove the test case as it's tested as is >> in setpgid03. Even the comments for the EPERM cases are identical in >> meaning. > I don't want to add an ultimate answer (not sure myself), but IMHO these > setpgid03.c and setpgid02.c aren't the same, because setpgid03.c calls: > 1) the fork() you mentioned > 2) setsid() (via SAFE_SETSID()) > > Therefore the EPERM meaning is the same, IMHO the code path in kernel and libc > is not the same. I see, that's fair. I would tend to agree then and leave it just for the potential difference in coverage. >> If it is to be kept, I think it could be better to use the kernel pid_max >> rather than >> an hardcoded value (for example 99999 would be possible on my machine), but >> I agree it would be fine. > Based to f2797fa44 commit message and my memory I guess Avinesh used PID 1 as > that's 100% sure it's different from whatever process group could LTP test have. > But IMHO that's not necessary, because PGID of both setpgid02 processes is > always the same as PID: > > $ ps xao user,pid,ppid,pgid,sid,comm | grep -e ^USER -e setpgid02 > USER PID PPID PGID SID COMMAND > pevik 1822063 1820900 1822063 1820900 setpgid02 > pevik 1822064 1822063 1822064 1820900 setpgid02 > > Therefore any PID would work => sure, scanning /proc/sys/kernel/pid_max LGTM: > SAFE_FILE_SCANF("/proc/sys/kernel/pid_max", "%lu", &pid_max); If there are no objections I will send a patch in the coming days then, thanks. > >> Adding Petr Vorel to CCs as he reviewed Avinesh's patch. > Thanks! I already posted my review, but missed following discussion. > > Kind regards, > Petr No worries, thanks for chiming in. Best regards Téo > >>> Thanks, >>> Edward >> Thanks for coming back to me, >> Best regards >> Téo
Hi Edward, > On Android, init does not setpgid, so getpgid(1) returns 0 and the third > case of setting pgid to a different session's process group does not > behave as expected: setpgid treats 0 as setting the pgid to the pid. > Instead, replace SAFE_GETPGID(1) with the expected value of 1. IMHO this was in the end fixed by 105b0fe17 ("setpgid02: Use pid_max as PGID for EPERM") therefore I'm closing this in patchwork as Rejected. Please let me know if there are still some problems on Android. Kind regards, Petr
diff --git a/testcases/kernel/syscalls/setpgid/setpgid02.c b/testcases/kernel/syscalls/setpgid/setpgid02.c index 4b63afee8..bf7b3176b 100644 --- a/testcases/kernel/syscalls/setpgid/setpgid02.c +++ b/testcases/kernel/syscalls/setpgid/setpgid02.c @@ -44,7 +44,7 @@ static void setup(void) * Getting pgid of init/systemd process to use it as a * process group from a different session for EPERM test */ - init_pgid = SAFE_GETPGID(1); + init_pgid = 1; } static void run(unsigned int n)
On Android, init does not setpgid, so getpgid(1) returns 0 and the third case of setting pgid to a different session's process group does not behave as expected: setpgid treats 0 as setting the pgid to the pid. Instead, replace SAFE_GETPGID(1) with the expected value of 1. Signed-off-by: Edward Liaw <edliaw@google.com> --- testcases/kernel/syscalls/setpgid/setpgid02.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)