Re: [PATCH] pasta: Workaround: wait for execvp() to be done in child before entering netns

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2954 bytes --]

On Fri, Nov 04, 2022 at 02:53:28AM +0100, Stefano Brivio wrote:
> This happens about every third time on the two_guests/basic test, and
> on that test only: we clone() twice, first to spawn a child, then to
> spawn a thread to check that we can enter the target network
> namespace.
> 
> In this thread, we open a file descriptor associated to the target
> namespace. It might happen that it doesn't exist yet: the kernel can
> legitimately take its time to create one, after clone(). In this
> case, at least on a 5.15 Linux kernel, trying to open that file again
> always yields EACCES, and we get stuck there.
> 
> This only occurs if we spawn two instances of pasta very close
> together, as it's done in the two_guests/basic case.
> 
> I couldn't figure out what the race condition is, yet, and especially
> if it's a kernel issue or something we're doing wrong. However, if we
> wait until the execvp() in the child is done, the issue disappears.
> I'm not sure yet if it's just because of timing and this is hiding an
> unrelated race condition.
> 
> The workaround consists of checking /proc/PID/exe against our own. If
> it's different, that means execvp() already completed and we can
> proceed. It's rather ugly, but much better than the alternative.
> Leave a FIXME there for the moment being.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Weird and ugly, but seems like we need it.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  pasta.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/pasta.c b/pasta.c
> index db86317..36072b2 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -81,9 +81,26 @@ void pasta_child_handler(int signal)
>   */
>  static int pasta_wait_for_ns(void *arg)
>  {
> +	char ns_exe_link[PATH_MAX], ns[PATH_MAX];
>  	struct ctx *c = (struct ctx *)arg;
>  	int flags = O_RDONLY | O_CLOEXEC;
> -	char ns[PATH_MAX];
> +	char exe[PATH_MAX] = { 0 };
> +
> +	/* FIXME: Why do we have to wait until execvp() is done in the child?
> +	 * If we don't, and the first call to open() below returns ENOENT, any
> +	 * subsequent call to it returns EACCES, at least on Linux 5.15, even
> +	 * though the observed PID is correct, and another process can open that
> +	 * path, and call setns() on that.
> +	 */
> +	snprintf(ns_exe_link, PATH_MAX, "/proc/%i/exe", pasta_child_pid);
> +	if (readlink("/proc/self/exe", exe, PATH_MAX - 1) != -1) {
> +		char ns_exe[PATH_MAX] = { 0 };
> +
> +		do {
> +			if (readlink(ns_exe_link, ns_exe, PATH_MAX - 1) == -1)
> +				break;
> +		} while (!strncmp(exe, ns_exe, PATH_MAX - 1));
> +	}
>  
>  	snprintf(ns, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid);
>  	do

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]