From 4987a057b4c4741e6a79eaa40ff38528f05b48e0 Mon Sep 17 00:00:00 2001 From: Kevin Parsons Date: Tue, 10 Nov 2020 23:43:22 -0800 Subject: [PATCH] Force disable VSMB direct map when the volume does not support it VSMB direct map requires support for querying FileIdInfo from the backing volume. There is a bug in certain Windows versions where instead of falling back to non-direct map when FileIdInfo is not supported, VSMB instead causes errors whenever files on the share are accessed. To work around this until the issue is fixed, we will query FileIdInfo ourselves when setting up a VSMB share, and force disable direct map if the query fails. Signed-off-by: Kevin Parsons --- internal/uvm/vsmb.go | 81 +++++++++++++++++++++++++++++++++++ internal/winapi/filesystem.go | 49 +++++++++++++++++++++ osversion/windowsbuilds.go | 3 ++ 3 files changed, 133 insertions(+) diff --git a/internal/uvm/vsmb.go b/internal/uvm/vsmb.go index 8c3616ef8c..9ecbbb1c6d 100644 --- a/internal/uvm/vsmb.go +++ b/internal/uvm/vsmb.go @@ -6,9 +6,15 @@ import ( "os" "path/filepath" "strconv" + "unsafe" + "github.com/Microsoft/hcsshim/internal/log" "github.com/Microsoft/hcsshim/internal/requesttype" hcsschema "github.com/Microsoft/hcsshim/internal/schema2" + "github.com/Microsoft/hcsshim/internal/winapi" + "github.com/Microsoft/hcsshim/osversion" + "github.com/sirupsen/logrus" + "golang.org/x/sys/windows" ) const vsmbSharePrefix = `\\?\VMSMB\VSMB-{dcc079ae-60ba-4d07-847c-3493609c0870}\` @@ -57,6 +63,67 @@ func (uvm *UtilityVM) findVSMBShare(ctx context.Context, m map[string]*VSMBShare return share, nil } +// openHostPath opens the given path and returns the handle. The handle is opened with +// full sharing and no access mask. The directory must already exist. This +// function is intended to return a handle suitable for use with GetFileInformationByHandleEx. +// +// We are not able to use builtin Go functionality for opening a directory path: +// - os.Open on a directory returns a os.File where Fd() is a search handle from FindFirstFile. +// - syscall.Open does not provide a way to specify FILE_FLAG_BACKUP_SEMANTICS, which is needed to +// open a directory. +// We could use os.Open if the path is a file, but it's easier to just use the same code for both. +// Therefore, we call windows.CreateFile directly. +func openHostPath(path string) (windows.Handle, error) { + u16, err := windows.UTF16PtrFromString(path) + if err != nil { + return 0, err + } + h, err := windows.CreateFile( + u16, + 0, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_FLAG_BACKUP_SEMANTICS, + 0) + if err != nil { + return 0, &os.PathError{ + Op: "CreateFile", + Path: path, + Err: err, + } + } + return h, nil +} + +// In 19H1, a change was made to VSMB to require querying file ID for the files being shared in +// order to support direct map. This change was made to ensure correctness in cases where direct +// map is used with saving/restoring VMs. +// +// However, certain file systems (such as Azure Files SMB shares) don't support the FileIdInfo +// query that is used. Azure Files in particular fails with ERROR_INVALID_PARAMETER. This issue +// affects at least 19H1, 19H2, 20H1, and 20H2. +// +// To work around this, we attempt to query for FileIdInfo ourselves if on an affected build. If +// the query fails, we override the specified options to force no direct map to be used. +func forceNoDirectMap(path string) (bool, error) { + if ver := osversion.Get().Build; ver < osversion.V19H1 || ver > osversion.V20H2 { + return false, nil + } + h, err := openHostPath(path) + if err != nil { + return false, err + } + defer windows.CloseHandle(h) + var info winapi.FILE_ID_INFO + // We check for any error, rather than just ERROR_INVALID_PARAMETER. It seems better to also + // fall back if e.g. some other backing filesystem is used which returns a different error. + if err := windows.GetFileInformationByHandleEx(h, winapi.FileIdInfo, (*byte)(unsafe.Pointer(&info)), uint32(unsafe.Sizeof(info))); err != nil { + return true, nil + } + return false, nil +} + // AddVSMB adds a VSMB share to a Windows utility VM. Each VSMB share is ref-counted and // only added if it isn't already. This is used for read-only layers, mapped directories // to a container, and for mapped pipes. @@ -88,6 +155,14 @@ func (uvm *UtilityVM) AddVSMB(ctx context.Context, hostPath string, options *hcs options.SingleFileMapping = true } hostPath = filepath.Clean(hostPath) + + if force, err := forceNoDirectMap(hostPath); err != nil { + return nil, err + } else if force { + log.G(ctx).WithField("path", hostPath).Info("Forcing NoDirectmap for VSMB mount") + options.NoDirectmap = true + } + var requestType = requesttype.Update shareKey := getVSMBShareKey(hostPath, options.ReadOnly) share, err := uvm.findVSMBShare(ctx, m, shareKey) @@ -113,6 +188,12 @@ func (uvm *UtilityVM) AddVSMB(ctx context.Context, hostPath string, options *hcs // isn't set (e.g. if used on an unrestricted share). So we only call Modify // if we are either doing an Add, or if RestrictFileAccess is set. if requestType == requesttype.Add || options.RestrictFileAccess { + log.G(ctx).WithFields(logrus.Fields{ + "name": share.name, + "path": hostPath, + "options": fmt.Sprintf("%+#v", options), + "operation": requestType, + }).Info("Modifying VSMB share") modification := &hcsschema.ModifySettingRequest{ RequestType: requestType, Settings: hcsschema.VirtualSmbShare{ diff --git a/internal/winapi/filesystem.go b/internal/winapi/filesystem.go index ab5daea782..490576b942 100644 --- a/internal/winapi/filesystem.go +++ b/internal/winapi/filesystem.go @@ -31,6 +31,43 @@ const ( STATUS_NO_MORE_ENTRIES = 0x8000001a ) +// Select entries from FILE_INFO_BY_HANDLE_CLASS. +// +// C declaration: +// typedef enum _FILE_INFO_BY_HANDLE_CLASS { +// FileBasicInfo, +// FileStandardInfo, +// FileNameInfo, +// FileRenameInfo, +// FileDispositionInfo, +// FileAllocationInfo, +// FileEndOfFileInfo, +// FileStreamInfo, +// FileCompressionInfo, +// FileAttributeTagInfo, +// FileIdBothDirectoryInfo, +// FileIdBothDirectoryRestartInfo, +// FileIoPriorityHintInfo, +// FileRemoteProtocolInfo, +// FileFullDirectoryInfo, +// FileFullDirectoryRestartInfo, +// FileStorageInfo, +// FileAlignmentInfo, +// FileIdInfo, +// FileIdExtdDirectoryInfo, +// FileIdExtdDirectoryRestartInfo, +// FileDispositionInfoEx, +// FileRenameInfoEx, +// FileCaseSensitiveInfo, +// FileNormalizedNameInfo, +// MaximumFileInfoByHandleClass +// } FILE_INFO_BY_HANDLE_CLASS, *PFILE_INFO_BY_HANDLE_CLASS; +// +// Documentation: https://docs.microsoft.com/en-us/windows/win32/api/minwinbase/ne-minwinbase-file_info_by_handle_class +const ( + FileIdInfo = 18 +) + type FileDispositionInformationEx struct { Flags uintptr } @@ -59,3 +96,15 @@ type FileLinkInformation struct { FileNameLength uint32 FileName [1]uint16 } + +// C declaration: +// typedef struct _FILE_ID_INFO { +// ULONGLONG VolumeSerialNumber; +// FILE_ID_128 FileId; +// } FILE_ID_INFO, *PFILE_ID_INFO; +// +// Documentation: https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_id_info +type FILE_ID_INFO struct { + VolumeSerialNumber uint64 + FileID [16]byte +} diff --git a/osversion/windowsbuilds.go b/osversion/windowsbuilds.go index 63d5ff0236..e9267b9554 100644 --- a/osversion/windowsbuilds.go +++ b/osversion/windowsbuilds.go @@ -32,4 +32,7 @@ const ( // V20H1 (version 2004) corresponds to Windows Server 2004 (semi-annual // channel). V20H1 = 19041 + + // V20H2 corresponds to Windows Server 20H2 (semi-annual channel). + V20H2 = 19042 )