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

[Petr Vorel <pvorel@suse.cz>]

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