Repository: moby/moby (language: go)

Issue:
vfs storage driver does not work on NFS
### Description

vfs storage driver does not work on NFS.

A simple 'docker run -it alpine' results in the following error:
docker: Error response from daemon: operation not supported.
level=error msg="Handler for POST /v1.42/containers/create returned error: operation not supported"

Using strace and what I got is:
lgetxattr("/var/lib/docker/vfs/dir/a93d6acc41f0fddc597f35fa1fb0b1c1b79c8ab04000570473cd15da20131cf3", "\
security.capability", 0xc000f3b200, 128) = -1 EOPNOTSUPP (Operation not supported)

This means it's trying to get extended security attributes but the underlying NFS does not support it.
Is this expected or is this a bug?


### Reproduce

docker run -it alpine

### Expected behavior

docker run succeeds

### docker version

```bash
# docker version
Client:
 Version:           20.10.21-ce
 API version:       1.41
 Go version:        go1.20.1
 Git commit:        baeda1f82a
 Built:             Thu Apr 27 02:36:59 2023
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server:
 Engine:
  Version:          20.10.21-ce
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.20.1
  Git commit:       4ed81ac0e2-unsupported
  Built:            Wed Nov  9 03:13:48 2022
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          v1.7.0-11-g6ea9bc57f.m
  GitCommit:        6ea9bc57f97cd6bdd62afe8c8295706de36afd51.m
 runc:
  Version:          1.1.5+dev
  GitCommit:        v1.1.5-1-g17a2d451-dirty
 docker-init:
  Version:          0.19.0
  GitCommit:        b9f42a0-dirty
```


### docker info

```bash
# docker info
Client:
 Context:    default
 Debug Mode: false

Server:
 Containers: 0
  Running: 0
  Paused: 0
  Stopped: 0
 Images: 0
 Server Version: 20.10.21-ce
 Storage Driver: vfs
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Cgroup Version: 1
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 io.containerd.runtime.v1.linux runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 6ea9bc57f97cd6bdd62afe8c8295706de36afd51.m
 runc version: v1.1.5-1-g17a2d451-dirty
 init version: b9f42a0-dirty
 Kernel Version: 5.15.103-yocto-standard
 Operating System: Poky (Yocto Project Reference Distro) 4.2 (mickledore)
 OSType: linux
 Architecture: x86_64
 CPUs: 4
 Total Memory: 3.841GiB
 Name: qemux86-64
 ID: PRTU:AZVE:RYAU:RIET:2XA2:AEPF:RJAZ:XFVO:XZEC:SOS4:WH2P:E776
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false
```


### Additional Info

The above 'docker version' & 'docker info' output are about 20.10.21, but I want to clarify that this issue has been is still there on current docker.

On docker 23.0.2, we got this problem.
On docker 20.10.21, we got this problem.
On docker 20.10.17, there's no such problem.
Allow for xattr copy failure for vfs
vfs is declared to work with any filesystem, but after https://github.com/moby/moby/commit/31f654a704f61768828d5950a13f30bb493d1239 it's no longer working with NFS.

As the extended attribute support depends on filesystem and if we do copy it in vfs and do not allow failure, that would essentially mean that vfs does NOT support all filesystems but only those that support xattr.

So we should just try to copy security.capabilities and allow for failure. In this way, vfs come back to the state of being able to run on any filesystem as declared in https://docs.docker.com/storage/storagedriver/select-storage-driver/.

Fixes https://github.com/moby/moby/issues/45417

<!--
Please make sure you've read and understood our contributing guidelines;
https://github.com/moby/moby/blob/master/CONTRIBUTING.md

** Make sure all your commits include a signature generated with `git commit -s` **

For additional information on our contributing process, read our contributing
guide https://docs.docker.com/opensource/code/

If this is a bug fix, make sure your description includes "fixes #xxxx", or
"closes #xxxx"

Please provide the following information:
-->

**- What I did**

**- How I did it**

**- How to verify it**

**- Description for the changelog**
<!--
Write a short (one line) summary that describes the changes in this
pull request for inclusion in the changelog:
-->


**- A picture of a cute animal (not mandatory but encouraged)**




Discussion / hints:
https://docs.docker.com/storage/storagedriver/select-storage-driver/
The above link says 'vfs' supports 'any filesystem'. This does not seem to be the case any more. It currently does not support NFS.
Or am I missing something?
The commit that introduces this problem is: https://github.com/moby/moby/commit/31f654a704f61768828d5950a13f30bb493d1239
The commit (https://github.com/moby/moby/commit/31f654a704f61768828d5950a13f30bb493d1239) is changing the semantics of DirCopy but it did not change daemon/graphdriver/vfs/copy_linux.go's call to DirCopy.
NFS v4.2 on Linux 5.9 and above support extended attributes when the underlying filesystem of the export does.

https://www.phoronix.com/news/Linux-5.9-NFS-Server-User-Xattr
https://www.phoronix.com/news/Linux-5.9-NFS-Client-Changes

Allowing the VFS driver to operate on filesystems which do not support xattrs will seemingly work at first but will cause hard-to-diagnose problems for containers which depend on [file capabilities](https://man7.org/linux/man-pages/man7/capabilities.7.html#:~:text=a%20capability%20set.-,File%20capabilities,-Since%20kernel%202.6.24) for proper functioning. The source of the "regression" is a fix for a real problem which affects real containers.
- #42655

In my opinion, the only bugs here are in the documentation and a lack of a filesystem-compatibility check by the vfs driver on daemon startup.
> NFS v4.2 on Linux 5.9 and above support extended attributes when the underlying filesystem of the export does.
> 
> https://www.phoronix.com/news/Linux-5.9-NFS-Server-User-Xattr https://www.phoronix.com/news/Linux-5.9-NFS-Client-Changes
> 

According to the above link, it only supports user xattr, not security xattr which is copied by vfs.
Anyway, even if new kernel & nfs suports security xattr, it's not favorable to require people to upgrade their testing infrastructure because such upgrade will usually comes with some chaos and chaos means cost.
If I ask people to upgrade the testing infrasture and tell them that the failure is from xattr copying, the next question I would get is: does your image have xattr?

> Allowing the VFS driver to operate on filesystems which do not support xattrs will seemingly work at first but will cause hard-to-diagnose problems for containers which depend on [file capabilities](https://man7.org/linux/man-pages/man7/capabilities.7.html#:~:text=a%20capability%20set.-,File%20capabilities,-Since%20kernel%202.6.24) for proper functioning. The source of the "regression" is a fix for a real problem which affects real containers.
> 
> * [VFS storage driver drops all file capabilities when creating a container #42655](https://github.com/moby/moby/issues/42655)
> 
> In my opinion, the only bugs here are in the documentation and a lack of a filesystem-compatibility check by the vfs driver on daemon startup.

In the PR's comments, we all agree with the daemon option solution. I'll do it. I'm not familiar with golang and docker source codes. So that may take some time. Please help review when it's done. Thanks :)


> NACKing for design review. The change which introduced this "regression" fixed a real bug in the VFS driver:
> 
> * [VFS storage driver drops all file capabilities when creating a container #42655](https://github.com/moby/moby/issues/42655)
> 
> Allowing the VFS driver to work on filesystems which do not support xattrs will seemingly work at first but will cause hard-to-diagnose problems for containers which depend on [file capabilities](https://man7.org/linux/man-pages/man7/capabilities.7.html#:~:text=a%20capability%20set.-,File%20capabilities,-Since%20kernel%202.6.24) for proper functioning.

@corhere I know the previous commit solved a real problem for images with xattrs. However, at the same time, it also introduced a real regression for images with no xattrs. How can an image fail for xattr copying when it does not have any?

@AkihiroSuda Daemon option is a good idea. I'll do it :)

Fix patch:
diff --git a/daemon/graphdriver/copy/copy.go b/daemon/graphdriver/copy/copy.go
index 0fb8a1a9d955e..8136b7a172686 100644
--- a/daemon/graphdriver/copy/copy.go
+++ b/daemon/graphdriver/copy/copy.go
@@ -5,6 +5,7 @@ package copy // import "github.com/docker/docker/daemon/graphdriver/copy"
 
 import (
 	"container/list"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -90,6 +91,11 @@ func legacyCopy(srcFile io.Reader, dstFile io.Writer) error {
 func copyXattr(srcPath, dstPath, attr string) error {
 	data, err := system.Lgetxattr(srcPath, attr)
 	if err != nil {
+		if errors.Is(err, syscall.EOPNOTSUPP) {
+			// Task failed successfully: there is no xattr to copy
+			// if the source filesystem doesn't support xattrs.
+			return nil
+		}
 		return err
 	}
 	if data != nil {


Test patch: