Starting parallel containers with same mounts can cause an error

[tonistiigi]

Description

When starting a container, runc will create destination paths for mountpoints. If there is another process creating the same paths at the same time then this step can fail with mkdir failing with EEXIST and container failing to start.

This race can happen if the paths getting mounted get modified externally, but it can also happen when just two runc processes are started at the same time and they compete with each other when creating the mountpoints.

I didn't look too deeply, but looks like a bug in mkdirp logic where it could just verify that the path already exists after it receives this error.

Reported via moby/buildkit#5566

Steps to reproduce the issue

This Dockerfile+script demonstrates the issue. I couldn't repro locally, but reproduces quite reliably (<20 iterations) in Github codespaces.

FROM alpine AS b1
RUN --mount=type=cache,target=/foo/bar --mount=type=cache,target=/foo/bar/baz echo 1

FROM alpine AS b2
RUN --mount=type=cache,target=/foo/bar --mount=type=cache,target=/foo/bar/baz echo 2

FROM scratch
COPY --from=b1 /etc/passwd p1
COPY --from=b2 /etc/passwd p2

#!/usr/bin/env bash

set -ex

for i in $(seq 1 100); do
docker buildx build --no-cache .
done

Note that in this Dockerfile, the paths for type=cache mounts are shared between stages. So /foo/bar points to same directory in both and failure happens when two runc processes both try to create /foo/bar/baz under it.

Describe the results you received and expected

 => ERROR [b2 2/2] RUN --mount=type=cache,target=/foo/bar --mount=type=cache,target=/foo/bar/baz echo 2                                   0.3s
------                                                                                                                                         
 > [b2 2/2] RUN --mount=type=cache,target=/foo/bar --mount=type=cache,target=/foo/bar/baz echo 2:                                              
0.275 runc run failed: unable to start container process: error during container init: error mounting "/var/lib/docker/overlay2/k72e3l8s3aw1xwilhrtrirlug/diff" to rootfs at "/foo/bar/baz": create mount destination for /foo/bar/baz mount: mkdirat /var/lib/docker/buildkit/executor/sjac66hcl5qvqkt3xav7q855m/rootfs/foo/bar/baz: file exists
------
Dockerfile:5
--------------------
   3 |     
   4 |     FROM alpine AS b2
   5 | >>> RUN --mount=type=cache,target=/foo/bar --mount=type=cache,target=/foo/bar/baz echo 2
   6 |     
   7 |     FROM scratch
--------------------
ERROR: failed to solve: process "/bin/sh -c echo 2" did not complete successfully: exit code: 1

What version of runc are you using?

1.1.14 and 1.2.2 in codespaces

Host OS information

No response

Host kernel information

No response

[cyphar]

If mkdirat(2) fails we could try to open the directory as a fallback, but the problem is that this would also be fragile to an attacker that is creating/deleting the directory. Either we would need to error out if the openat(2) fails, or we could get caught in a DoS if we do a retry loop instead. I guess for your use-case you don't care if we just fail if the fallback openat(2) fails?

(This would be a bug in https://github.com/cyphar/filepath-securejoin FWIW. I'll open a bug there.)

[tonistiigi]

also be fragile to an attacker that is creating/deleting the directory

If the external modifier deletes the directory after runc has called stat on it then I think erroring is fine. But two runc trying to recreate same dir at the same time should be allowed. The actual mount/stat checks should still take care of there not being any breakouts. I think it is fine even if external modification of switching between directory and symlink is causing an error in an edge case.

Isn't it enough to just run a stat call after mkdirat returns EEXISTS to verify that all is ok with the path and it points to regular directory?

[cyphar]

If we just need to allow racing creates to not cause errors, we don't need a stat call (it also wouldn't help with races), the patch can just be something as simple as:

diff --git a/mkdir_linux.go b/mkdir_linux.go
index b5f674524c84..6dfe8c42b364 100644
--- a/mkdir_linux.go
+++ b/mkdir_linux.go
@@ -119,7 +119,12 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
                // NOTE: mkdir(2) will not follow trailing symlinks, so we can safely
                // create the final component without worrying about symlink-exchange
                // attacks.
-               if err := unix.Mkdirat(int(currentDir.Fd()), part, uint32(mode)); err != nil {
+               //
+               // If we get -EEXIST, it's possible that another program created the
+               // directory at the same time as us. In that case, just continue on as
+               // if we created it (if the created inode is not a directory, the
+               // following open call will fail).
+               if err := unix.Mkdirat(int(currentDir.Fd()), part, uint32(mode)); err != nil && !errors.Is(err, unix.EEXIST) {
                        err = &os.PathError{Op: "mkdirat", Path: currentDir.Name() + "/" + part, Err: err}
                        // Make the error a bit nicer if the directory is dead.
                        if err2 := isDeadInode(currentDir); err2 != nil {

(The following openat(O_DIRECTORY|O_NOFOLLOW) acts like a "is this a directory" check, but it's atomic.)

I'll open a PR with this, along with some extra tests.

[tonistiigi]

Ah, ok. I didn't check that this mkdirat was already followed by openat.

[cyphar]

cyphar/filepath-securejoin#35 should fix the issue. I'll merge it and do a new release soon.