| Message ID | 20230307114507.332309-4-rodrigo@sdfg.com.ar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tests for idmapped tmpfs | expand |
On Tue, Mar 07, 2023 at 12:44:59PM +0100, Rodrigo Campos wrote: > Talking with Christian Brauner about a different problem, he mentioned > that technically this race condition exists and we should fix it. > > The race is that when we clone, we call a function that just returns > while at the same time we try to get the userns via /proc/pid/ns/user. > The thing is that, while the pid needs to be reaped, Christian said that > the userns file cease to exist as soon as the program finishes. See exit_task_namespaces() in kernel/exit.c:do_exit(). > > So, let's make the function never return, so we always can get the > userns. We are already sending a SIGKILL to this pid, so nothing else > remaining to not leak the process. > > Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar> > --- > src/vfs/utils.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git src/vfs/utils.c src/vfs/utils.c > index ea7536c1..67779e83 100644 > --- src/vfs/utils.c > +++ src/vfs/utils.c > @@ -58,9 +58,10 @@ pid_t do_clone(int (*fn)(void *), void *arg, int flags) > #endif > } > > -static int get_userns_fd_cb(void *data) > +__attribute__((noreturn)) static int get_userns_fd_cb(void *data) > { > - return 0; > + for (;;) > + pause(); Should this add a _exit(0)? It's pretty odd otherwise. And do we need noreturn?
On 3/7/23 17:46, Christian Brauner wrote: > On Tue, Mar 07, 2023 at 12:44:59PM +0100, Rodrigo Campos wrote: >> Talking with Christian Brauner about a different problem, he mentioned >> that technically this race condition exists and we should fix it. >> >> The race is that when we clone, we call a function that just returns >> while at the same time we try to get the userns via /proc/pid/ns/user. >> The thing is that, while the pid needs to be reaped, Christian said that >> the userns file cease to exist as soon as the program finishes. > > See exit_task_namespaces() in kernel/exit.c:do_exit(). Cool, thanks! Added that instead, then :) >> So, let's make the function never return, so we always can get the >> userns. We are already sending a SIGKILL to this pid, so nothing else >> remaining to not leak the process. >> >> Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar> >> --- >> src/vfs/utils.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git src/vfs/utils.c src/vfs/utils.c >> index ea7536c1..67779e83 100644 >> --- src/vfs/utils.c >> +++ src/vfs/utils.c >> @@ -58,9 +58,10 @@ pid_t do_clone(int (*fn)(void *), void *arg, int flags) >> #endif >> } >> >> -static int get_userns_fd_cb(void *data) >> +__attribute__((noreturn)) static int get_userns_fd_cb(void *data) >> { >> - return 0; >> + for (;;) >> + pause(); > > Should this add a _exit(0)? It's pretty odd otherwise. And do we need > noreturn? Agree, let's do that and remove the attribute.
diff --git src/vfs/utils.c src/vfs/utils.c index ea7536c1..67779e83 100644 --- src/vfs/utils.c +++ src/vfs/utils.c @@ -58,9 +58,10 @@ pid_t do_clone(int (*fn)(void *), void *arg, int flags) #endif } -static int get_userns_fd_cb(void *data) +__attribute__((noreturn)) static int get_userns_fd_cb(void *data) { - return 0; + for (;;) + pause(); } int wait_for_pid(pid_t pid)
Talking with Christian Brauner about a different problem, he mentioned that technically this race condition exists and we should fix it. The race is that when we clone, we call a function that just returns while at the same time we try to get the userns via /proc/pid/ns/user. The thing is that, while the pid needs to be reaped, Christian said that the userns file cease to exist as soon as the program finishes. So, let's make the function never return, so we always can get the userns. We are already sending a SIGKILL to this pid, so nothing else remaining to not leak the process. Signed-off-by: Rodrigo Campos <rodrigo@sdfg.com.ar> --- src/vfs/utils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)