For writing what?

If you do not intend to write anything, you may just need to append these flags

https://github.com/moby/moby/blob/v20.10.5/daemon/oci_linux.go#L420-L450

Thanks @AkihiroSuda , with help from you I found the root cause of the failure.
In linux/fs/namespace.c:do_remount(), it checks the remount operation doesn't reset (instead of modifying) RO,NOSUID,NODEV,NOEXEC and MNT_ATIME_xxx.

        /* Don't allow changing of locked mnt flags.
         *
         * No locks need to be held here while testing the various
         * MNT_LOCK flags because those flags can never be cleared
         * once they are set.
         */
        if ((mnt->mnt.mnt_flags & MNT_LOCK_READONLY) &&
            !(mnt_flags & MNT_READONLY)) {
                return -EPERM;
        }
        if ((mnt->mnt.mnt_flags & MNT_LOCK_NODEV) &&
            !(mnt_flags & MNT_NODEV)) {
                return -EPERM;
        }
        if ((mnt->mnt.mnt_flags & MNT_LOCK_NOSUID) &&
            !(mnt_flags & MNT_NOSUID)) {
                return -EPERM;
        }
        if ((mnt->mnt.mnt_flags & MNT_LOCK_NOEXEC) &&
            !(mnt_flags & MNT_NOEXEC)) {
                return -EPERM;
        }
        if ((mnt->mnt.mnt_flags & MNT_LOCK_ATIME) &&
            ((mnt->mnt.mnt_flags & MNT_ATIME_MASK) != (mnt_flags & MNT_ATIME_MASK))) {
                return -EPERM;
        }

Originally the /dev/shm was mount as
shm on /dev/shm type tmpfs (rw,nosuid,nodev,noexec,relatime,size=65536k).

So when doing in bind mounting in pause container with user namespace enabled, in addition to changing rw to ro, we still need those "nosuid,nodev,noexec,relatime" flags. With those flags, it succeeds:

root@f8ff64bfc7d8:/kata# mount -o bind,ro,nosuid,nodev,noexec,relatime /dev/shm /dev/shm
root@f8ff64bfc7d8:/kata# mount -o bind,ro /dev/shm /dev/shm
mount: /dev/shm: filesystem was mounted, but any subsequent operation failed: Unknown error 5005.

So we have a better solution now and will update the patch.

One more information for the root cause. When the mount flags contains "BIND | READONLY", it will cause a second mount syscall with "mount_flags | REMOUNT", which will handled by do_remount(), though causes the failure. By changing "ro" to "rw", the second mount syscall will not happen, so it succeeds.

This is not needed AFAIK. IIRC no other part of this function is doing the "remove X first", in case it was added. Here we are constructing it for the first time.

It's an optimization to avoid two tmpfs filesystems for /dev/shm.

@jiangliu optimization?

I don't follow... oci.WithoutMounts is this code. This just removes the related mount. I'm saying that mount should not be there, because we are creating it for the first time in this function. Removing something that should not be there is not an optimization, but the opposite ;)

Also, this WithoutXXX pattern is also not used in other parts of this function.

Do we really need this? Am I missing something?

It's really an optimization:) And a little complex to tell the story.
The first instance of /dev/shm is added along the call stack:

criService::sandboxContainerSpec() -> criService::runtimeSpec() -> oci.GenerateSpec -> oci.generateDefaultSpecWithPlatform() -> oci.populateDefaultUnixSpec() -> oci.defaultMounts() ->
		{
			Destination: "/dev/shm",
			Type:        "tmpfs",
			Source:      "shm",
			Options:     []string{"nosuid", "noexec", "nodev", "mode=1777", "size=65536k"},
		},

The second instance is added by the code below.
So specOpts = append(specOpts, oci.WithoutMounts("/dev/shm")) is used to remove the first redundant copy.

Ohh, I see, thanks! I was looking too quickly and missed in the bug report that there are two instances listed.

Do we want to change this default for all the containers?

In my PoC branch I only changed this flags if userns is enabled. See here, for example: https://github.com/kinvolk/containerd/blob/54c1f3529d64b58e187ab0341b3b8f669be30a0d/pkg/cri/server/sandbox_run_linux.go#L122-L134

If we wait for the CRI changes, we can make this conditional on that being enabled.

I think it would be better to change it for all. Otherwise we will get an /dev/shm with:

shm on /dev/shm type tmpfs (ro,relatime,size=65536k)

which doesn't confirm to the runC spec(https://github.com/opencontainers/runc/blob/main/libcontainer/SPEC.md):

/dev/shm | tmpfs | MS_NOEXEC,MS_NOSUID,MS_NODEV | mode=1777,size=65536k

@jiangliu Well, it is a backwards incompatible change, so IMHO we should put some thought into doing that change. At least, IMHO, it deserves to be highlighted in the PR description for open debate, and clear reasons to why it is worth doing it.

which doesn't confirm to the runC spec(https://github.com/opencontainers/runc/blob/main/libcontainer/SPEC.md):

Do we care about that? I mean, AFAIK we should care about the runtime-spec. That seems to be some documentation for the libcontainer and its defaults. It is good to know, but nothing else IMHO.

I have thought about the possible incompatible issue. We are creating env for the infra (pause) container, and the /dev/shm is mounted as ro instead of normal rw, that means the pause container won't write data to /dev/shm and it doesn't make sense for pause container to read data from /dev/shm too.

One the other hand, my first feeling is "aha, a possible attack path". Because the same tmpfs instance is mounted for both pause container in ro mode and app containers in ro,nosuid,nodev,noexec mode. If we could copy suid program to the shared tmpfs, and trick pause container to execute it, that's bad. After some experiments, it turns out that attack path is impossible. But I do feel it's better to mount /dev/shm in ro,nosuid,noexec,nodev mode for both pause and app containers.

BTW, we found this issue when analyzing the generated OCI runtime config file and then found the root cause.

https://github.com/kinvolk/containerd/blob/54c1f3529d64b58e187ab0341b3b8f669be30a0d/pkg/cri/server/sandbox_run_linux.go#L122-L134

For user ns enabled case, we could also enable the "ro" flag.

	sanboxDevShmOpt := []string{"rbind", "ro"}
	if userNS {
		// When userns is enabled, the mount fails if it doesn't include these options,
		// except for rprivate. rpivate is not required, but nice to have.
		sanboxDevShmOpt = append(sanboxDevShmOpt, "rprivate", "noexec", "nosuid", "nodev")
	}

Ok, so I guess due to the lack of comment on that part, that the runc/libcontainer thing is ok, right?

I'm in a rush for kubecon and to make sure the KEP makes it into 1.25. I don't have time to properly review and test this now. However, if this only affects the sandbox mount, it is probably not a big issue. I don't see any good reason to do it one way or the other. But it is still worth noting this change in the PR description and release notes IMHO.

This last sentence seems weird to me: those flags do have an effect without userns. What about this instead?

fixed
I am not a native speaker of English, thanks for your comments.