| Message ID | 20230827155746.84781-33-kariem.taha2.7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | bsd-user: Implement freebsd process related system calls. | expand |
On 8/27/23 08:57, Karim Taha wrote: > From: Stacey Son <sson@FreeBSD.org> > > Signed-off-by: Stacey Son <sson@FreeBSD.org> > Signed-off-by: Karim Taha <kariem.taha2.7@gmail.com> > --- > bsd-user/freebsd/os-proc.h | 32 ++++++++++++++++++++++++++++++++ > bsd-user/freebsd/os-syscall.c | 4 ++++ > 2 files changed, 36 insertions(+) > > diff --git a/bsd-user/freebsd/os-proc.h b/bsd-user/freebsd/os-proc.h > index 94824d737a..1eaba908a5 100644 > --- a/bsd-user/freebsd/os-proc.h > +++ b/bsd-user/freebsd/os-proc.h > @@ -248,4 +248,36 @@ static inline abi_long do_freebsd_rfork(void *cpu_env, abi_long flags) > > } > > +/* pdfork(2) */ > +static inline abi_long do_freebsd_pdfork(void *cpu_env, abi_ulong target_fdp, > + abi_long flags) > +{ > + abi_long ret; > + abi_ulong child_flag; > + int fd; > + > + fork_start(); > + ret = pdfork(&fd, flags); > + if (ret == 0) { > + /* child */ > + child_flag = 1; > + target_cpu_clone_regs(cpu_env, 0); > + } else { > + /* parent */ > + child_flag = 0; > + } > + if (put_user_s32(fd, target_fdp)) { > + return -TARGET_EFAULT; > + } I *think* this copy belongs in the parent? It's really hard to follow the path of new process creation within the freebsd kernel. Anyway, the rest looks fine so I'll give an Acked-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, Aug 29, 2023 at 2:58 PM Richard Henderson < richard.henderson@linaro.org> wrote: > On 8/27/23 08:57, Karim Taha wrote: > > From: Stacey Son <sson@FreeBSD.org> > > > > Signed-off-by: Stacey Son <sson@FreeBSD.org> > > Signed-off-by: Karim Taha <kariem.taha2.7@gmail.com> > > --- > > bsd-user/freebsd/os-proc.h | 32 ++++++++++++++++++++++++++++++++ > > bsd-user/freebsd/os-syscall.c | 4 ++++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/bsd-user/freebsd/os-proc.h b/bsd-user/freebsd/os-proc.h > > index 94824d737a..1eaba908a5 100644 > > --- a/bsd-user/freebsd/os-proc.h > > +++ b/bsd-user/freebsd/os-proc.h > > @@ -248,4 +248,36 @@ static inline abi_long do_freebsd_rfork(void > *cpu_env, abi_long flags) > > > > } > > > > +/* pdfork(2) */ > > +static inline abi_long do_freebsd_pdfork(void *cpu_env, abi_ulong > target_fdp, > > + abi_long flags) > > +{ > > + abi_long ret; > > + abi_ulong child_flag; > > + int fd; > > + > > + fork_start(); > > + ret = pdfork(&fd, flags); > > + if (ret == 0) { > > + /* child */ > > + child_flag = 1; > > + target_cpu_clone_regs(cpu_env, 0); > > + } else { > > + /* parent */ > > + child_flag = 0; > > + } > > + if (put_user_s32(fd, target_fdp)) { > > + return -TARGET_EFAULT; > > + } > > I *think* this copy belongs in the parent? I think that it's copied out in both cases. For normal fork, this would be 0 for the pid. However, it appears to return the same FD to both the parent and child (see your next comment), so it should be in both paths. And even if it returned something different for parent and child (which seems unlikely given how the code is setup), we want to return the fd each one sees. So either way, I think this code is correct. > It's really hard to follow the path of new > process creation within the freebsd kernel. > Agreed. > Anyway, the rest looks fine so I'll give an > > Acked-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Warner Losh <imp@bsdimp.com>
On 8/29/23 14:27, Warner Losh wrote: > > + if (put_user_s32(fd, target_fdp)) { > > + return -TARGET_EFAULT; > > + } > > I *think* this copy belongs in the parent? > > > I think that it's copied out in both cases. For normal fork, this would > be 0 for the pid. However, it appears to return the same FD to both > the parent and child (see your next comment), so it should be in both > paths. And even if it returned something different for parent and child > (which seems unlikely given how the code is setup), we want to return > the fd each one sees. So either way, I think this code is correct. > > It's really hard to follow the path of new > process creation within the freebsd kernel. > > > Agreed. I think that the child never returns from do_fork. The child pid == 0 happens as part of do_fork or vm_forkproc or somesuch, but the new process definitely begins life at fork_return. Therefore only the parent passes returns from fork1 to set *fdp. r~
On Tue, Aug 29, 2023 at 3:53 PM Richard Henderson < richard.henderson@linaro.org> wrote: > On 8/29/23 14:27, Warner Losh wrote: > > > + if (put_user_s32(fd, target_fdp)) { > > > + return -TARGET_EFAULT; > > > + } > > > > I *think* this copy belongs in the parent? > > > > > > I think that it's copied out in both cases. For normal fork, this would > > be 0 for the pid. However, it appears to return the same FD to both > > the parent and child (see your next comment), so it should be in both > > paths. And even if it returned something different for parent and child > > (which seems unlikely given how the code is setup), we want to return > > the fd each one sees. So either way, I think this code is correct. > > > > It's really hard to follow the path of new > > process creation within the freebsd kernel. > > > > > > Agreed. > > I think that the child never returns from do_fork. The child pid == 0 > happens as part of > do_fork or vm_forkproc or somesuch, but the new process definitely begins > life at fork_return. > I confused the 'returns twice' behavior in userland with the gymnastics the kernel does on creating a new process (where things don't return twice). Having looked at that code, I'm sure you are right now and it should only be set in the parent. I don't see where it is set in the fork_return path. For normal fork, the return value register is cleared, as is the carry bit, used to signal errors from system calls on FreeBSD. And having asked someone whose more of an expert, he confirms it is not set in the child. Therefore only the parent passes returns from fork1 to set *fdp. > I agree. We should move that code to the parent branch. Warner > > r~ >
diff --git a/bsd-user/freebsd/os-proc.h b/bsd-user/freebsd/os-proc.h index 94824d737a..1eaba908a5 100644 --- a/bsd-user/freebsd/os-proc.h +++ b/bsd-user/freebsd/os-proc.h @@ -248,4 +248,36 @@ static inline abi_long do_freebsd_rfork(void *cpu_env, abi_long flags) } +/* pdfork(2) */ +static inline abi_long do_freebsd_pdfork(void *cpu_env, abi_ulong target_fdp, + abi_long flags) +{ + abi_long ret; + abi_ulong child_flag; + int fd; + + fork_start(); + ret = pdfork(&fd, flags); + if (ret == 0) { + /* child */ + child_flag = 1; + target_cpu_clone_regs(cpu_env, 0); + } else { + /* parent */ + child_flag = 0; + } + if (put_user_s32(fd, target_fdp)) { + return -TARGET_EFAULT; + } + + /* + * The fork system call sets a child flag in the second return + * value: 0 for parent process, 1 for child process. + */ + set_second_rval(cpu_env, child_flag); + fork_end(child_flag); + + return ret; +} + #endif /* BSD_USER_FREEBSD_OS_PROC_H */ diff --git a/bsd-user/freebsd/os-syscall.c b/bsd-user/freebsd/os-syscall.c index 4464b3369c..27fc9d21fb 100644 --- a/bsd-user/freebsd/os-syscall.c +++ b/bsd-user/freebsd/os-syscall.c @@ -236,6 +236,10 @@ static abi_long freebsd_syscall(void *cpu_env, int num, abi_long arg1, ret = do_freebsd_rfork(cpu_env, arg1); break; + case TARGET_FREEBSD_NR_pdfork: /* pdfork(2) */ + ret = do_freebsd_pdfork(cpu_env, arg1, arg2); + break; + case TARGET_FREEBSD_NR_execve: /* execve(2) */ ret = do_freebsd_execve(arg1, arg2, arg3); break;