[LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Edward Liaw via ltp]

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)
-- 
2.40.0.348.gf938b09366-goog


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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Petr Vorel]

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

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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Avinesh Kumar]

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



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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Teo Couprie Diaz]

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


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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Edward Liaw via ltp]

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

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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Teo Couprie Diaz]

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

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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Petr Vorel]

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

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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Teo Couprie Diaz]

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

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

Re: [LTP] [PATCH v2] setpgid02: use 1 instead of getpgid(1)

[Petr Vorel]

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

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