[PATCH 0/3] fix unlink/rename failure in hyper-v container

[PATCH 0/3] fix unlink/rename failure in hyper-v container

[YO4]

Hello, cygwin developers.
I am using msys2 and it is based on the cygwin codebase.
I was working inside a windows container and encountered rm.exe and mv.exe failures.
I would be honored if you could merge my patch into upstream.

How to reproduce:
Use a version of windows that supports POSIX unlink/rename semantics as a container.
In a windows container using hyper-v isolation, rm.exe and mv.exe fail on bind mounted (also volume mounted) directories.

What this patch does
This patch will disable POSIX semantics and retry on the above failures.

The strace on failure is as follows
  806  192814 [main] rm 1473 _unlink_nt: Trying to delete \??\C:\binded_dir\file_to_unlink, isdir = 0
 1236  194050 [main] rm 1473 _unlink_nt: \??\C:\binded_dir\file_to_unlink, return status = 0xC000000D
  574  194624 [main] rm 1473 seterrno_from_nt_status: /c/S/msys2-runtime/src/msys2-runtime/winsup/cygwin/syscalls.cc:1120 status 0xC000000D -> windows error 87
  490  195114 [main] rm 1473 geterrno_from_win_error: windows error 87 == errno 22
  494  195608 [main] rm 1473 unlink: -1 = unlink(C:/binded_dir/file_to_unlink), errno 22

The strace with the patch is as follows
  737  234961 [main] rm 1822 _unlink_nt: Trying to delete \??\C:\binded_dir\file_to_unlink, isdir = 0
 1277  236238 [main] rm 1822 _unlink_nt: NtSetInformationFile (\??\C:\binded_dir\file_to_unlink, FileDispositionInformationEx) returns 0xC000000D with posix semantics. Disable it and retry.
 1411  237649 [main] rm 1822 _unlink_nt: \??\C:\binded_dir\file_to_unlink, return status = 0x0
  558  238207 [main] rm 1822 unlink: 0 = unlink(C:/binded_dir/file_to_unlink)

I ran the test in Appveyor so you can view the entire log at
https://ci.appveyor.com/project/YO4/test-msys2-in-container/builds/46532757/job/qinojh64uo6el78s
strace logs are available for download as artifacts.

This issue was first discussed here
https://github.com/msys2/msys2-runtime/issues/59
This patch is first available at 
https://github.com/msys2/msys2-runtime/pull/141


YO4 (3):
  fix unlink in container
  fix rename in container
  log disabling posix semantics

 winsup/cygwin/syscalls.cc | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

-- 
2.40.0.windows.1

[PATCH 1/3] fix unlink in container

[YO4]

Deleting files returns STATUS_INVALID_PARAMETE on a bind mounted file system in hyper-v container with FILE_DISPOSITION_POSIX_SEMANTICS.
Therefore fall back to default method.

This code is suggeted by dscho and I change it more simple.
---
 winsup/cygwin/syscalls.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 5c7f46a99..ac89888ce 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -731,7 +731,10 @@ _unlink_nt (path_conv &pc, bool shareable)
       /* Trying to delete in-use executables and DLLs using
          FILE_DISPOSITION_POSIX_SEMANTICS returns STATUS_CANNOT_DELETE.
 	 Fall back to the default method. */
-      if (status != STATUS_CANNOT_DELETE)
+      /* Additionaly that returns STATUS_INVALID_PARAMETER
+         on a bind mounted fs in hyper-v container. Falling back too. */
+      if (status != STATUS_CANNOT_DELETE
+          && status != STATUS_INVALID_PARAMETER)
 	goto out;
     }
 
-- 
2.40.0.windows.1

[PATCH 2/3] fix rename in container

[YO4]

Renaming files returns STATUS_INVALID_PARAMETE on a bind mounted file system in hyper-v container with FILE_RENAME_POSIX_SEMANTICS.

Disable the use_posix_semantics flag and retry.
---
 winsup/cygwin/syscalls.cc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index ac89888ce..69699c8aa 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -2438,6 +2438,7 @@ rename2 (const char *oldpath, const char *newpath, unsigned int at2flags)
 			    && !oldpc.isremote ()
 			    && oldpc.fs_is_ntfs ();
 
+ignore_posix_semantics_retry:
       /* Opening the file must be part of the transaction.  It's not sufficient
 	 to call only NtSetInformationFile under the transaction.  Therefore we
 	 have to start the transaction here, if necessary.  Don't start
@@ -2682,6 +2683,15 @@ skip_pre_W10_checks:
 	    unlink_nt (*removepc);
 	  res = 0;
 	}
+      else if (use_posix_semantics && status == STATUS_INVALID_PARAMETER)
+        {
+          /* NtSetInformationFile returns STATUS_INVALID_PARAMETER
+             on a bind mounted file system in hyper-v container
+             with FILE_RENAME_POSIX_SEMANTICS.
+             Disable the use_posix semntics flag and retry. */
+          use_posix_semantics = 0;
+          goto ignore_posix_semantics_retry;
+        }
       else
 	__seterrno_from_nt_status (status);
     }
-- 
2.40.0.windows.1

[PATCH 3/3] log disabling posix semantics

[YO4]

---
 winsup/cygwin/syscalls.cc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 69699c8aa..ca40681cd 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -736,6 +736,9 @@ _unlink_nt (path_conv &pc, bool shareable)
       if (status != STATUS_CANNOT_DELETE
           && status != STATUS_INVALID_PARAMETER)
 	goto out;
+      debug_printf ("NtSetInformationFile (%S, FileDispositionInformationEx)"
+                    " returns %y with posix semantics. Disable it and retry.",
+                    pc.get_nt_native_path (), status);
     }
 
   /* If the R/O attribute is set, we have to open the file with
@@ -2689,6 +2692,11 @@ skip_pre_W10_checks:
              on a bind mounted file system in hyper-v container
              with FILE_RENAME_POSIX_SEMANTICS.
              Disable the use_posix semntics flag and retry. */
+          debug_printf ("NtSetInformationFile "
+                        "(%S, %S, FileRenameInformationEx) failed "
+                        "with posix semantics. Disable it and retry.",
+                        oldpc.get_nt_native_path (),
+                        newpc.get_nt_native_path ());
           use_posix_semantics = 0;
           goto ignore_posix_semantics_retry;
         }
-- 
2.40.0.windows.1

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

Hi,

On Mar 17 23:43, YO4 wrote:
> Hello, cygwin developers.
> I am using msys2 and it is based on the cygwin codebase.
> I was working inside a windows container and encountered rm.exe and mv.exe failures.
> I would be honored if you could merge my patch into upstream.

The patchset looks basically ok, and I'm not opposed to apply it.
But I do wonder if we cant't come up with a better solution somehow.

The problem is that you will suffer another performance hit on top
of the fact that Cygwin is already slow anyway.  Every time you
run rm or rename, you will have to call the OS function twice.
So rm -r will become even slower.

The question here is, do we have a way to recognize a Hyper-V mount?

Have a look at fs_info::update() in mount.cc.  There's a lot of checking
for various filesystems and their quirks.  If we have a way to
distinguish a Hyper-V mount from a "normal" NTFS, we could add it as a
filesystem type of its own, and the "use_posix_semantics" flag would
simply never be set.

If we can do that, it's the cleaner solution, IMHO.

For that, can we start with you running 

  $ cd <your Hyper-V dir>
  $ /usr/lib/csih/getVolInfo .

and paste the output here?  If there's any chance we can recognize
a Hyper-V dir, we should take it.

Oh, and, btw., would you mind to recreate your git patches with your
real name, please?  I'm not hot on adding pseudonyms into the git
repo.  We all use our real names.  Also, a matching `Signed-Off-By:'
would be helpful.


Thanks,
Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Yoshinao Muramatsu]

Hi Corinna.
I'm Yoshinao Muramatsu. Thank you for your thoughtful guidance.
I have not modified the patch yet,
but I have made some observations and post the results.

I forgot to mention in my first post that there is a workaround
for this issue, which is to use process isolation unless
hyper-v isolation is absolutely necessary
(mainly caused by build number difference between host and container).

In the logs, here we can find some differences.
But I believe it is unclear if it will always be so.
If additional inspections are done, they will be costly.
I would also like to see better support for POSIX unlink/rename
semantics in the container mechanism.

Therefore, I think it is not a bad idea to simply retry rather than spending
time every time to check the conditions to deal with rare situations.
However, in certain situations it happens all the time.
(So if we can cache it that would be great.)
But performance in the vast majority of other situations is more important.

In the following logs, c:\ is the normal ntfs volume in the container,
andc:\opt is the volume of the bind mounted host.
Where the differences are
- 0x00000020 Supports Disk Quotas
- 0x01000000 Supports Open By FileID
- 0x02000000 Supports USN Journal

== hyper-v container (has issue)
$ mount
C:/cygwin64/bin on /usr/bin type ntfs (binary,auto)
C:/cygwin64/lib on /usr/lib type ntfs (binary,auto)
C:/cygwin64 on / type ntfs (binary,auto)
C: on /cygdrive/c type ntfs (binary,posix=0,user,noumount,auto)

$ /usr/lib/csih/getVolInfo.exe /cygdrive/c/
Device Type        : 7
Characteristics    : 20020
Volume Name        : <>
Serial Number      : 3456656850
Max Filenamelength : 255
Filesystemname     : <NTFS>
Flags              : 1c706df
...snip...

$ /usr/lib/csih/getVolInfo.exe /cygdrive/c/opt/
Device Type        : 7
Characteristics    : 20020
Volume Name        : <>
Serial Number      : 955187689
Max Filenamelength : 255
Filesystemname     : <NTFS>
Flags              : 2c706ff
...snip...

=== process container (works fine)
$ /usr/lib/csih/getVolInfo.exe /cygdrive/c
Device Type        : 7
Characteristics    : 20
Volume Name        : <>
Serial Number      : 3456656850
Max Filenamelength : 255
Filesystemname     : <NTFS>
Flags              : 1c706df
...snip...

$ /usr/lib/csih/getVolInfo.exe /cygdrive/c/opt
Device Type        : 7
Characteristics    : 20
Volume Name        : <>
Serial Number      : 955187689
Max Filenamelength : 255
Filesystemname     : <NTFS>
Flags              : 3e706ff
...snip...

=== host
Microsoft Windows [Version 10.0.20348.1547]
(c) Microsoft Corporation. All rights reserved.

C:\Users\Administrator>\cygpkgs\getVolInfo.exe c:\
Device Type        : 7
Characteristics    : 20020
Volume Name        : <>
Serial Number      : 955187689
Max Filenamelength : 255
Filesystemname     : <NTFS>
Flags              : 3e706ff
   FILE_CASE_SENSITIVE_SEARCH  : TRUE
   FILE_CASE_PRESERVED_NAMES   : TRUE
   FILE_UNICODE_ON_DISK        : TRUE
   FILE_PERSISTENT_ACLS        : TRUE
   FILE_FILE_COMPRESSION       : TRUE
   FILE_VOLUME_QUOTAS          : TRUE
   FILE_SUPPORTS_SPARSE_FILES  : TRUE
   FILE_SUPPORTS_REPARSE_POINTS: TRUE
   FILE_SUPPORTS_REMOTE_STORAGE: FALSE
   FILE_VOLUME_IS_COMPRESSED   : FALSE
   FILE_SUPPORTS_OBJECT_IDS    : TRUE
   FILE_SUPPORTS_ENCRYPTION    : TRUE
   FILE_NAMED_STREAMS          : TRUE
   FILE_READ_ONLY_VOLUME       : FALSE
   FILE_SEQUENTIAL_WRITE_ONCE  : FALSE
   FILE_SUPPORTS_TRANSACTIONS  : TRUE

C:\Users\Administrator>fsutil fsinfo volumeinfo c:
Volume Name :
Volume Serial Number : 0x38ef01e9
Max Component Length : 255
File System Name : NTFS
Is ReadWrite
Not Thinly-Provisioned
Supports Case-sensitive filenames
Preserves Case of filenames
Supports Unicode in filenames
Preserves & Enforces ACL's
Supports file-based Compression
Supports Disk Quotas
Supports Sparse files
Supports Reparse Points
Returns Handle Close Result Information
Supports POSIX-style Unlink and Rename
Supports Object Identifiers
Supports Encrypted File System
Supports Named Streams
Supports Transactions
Supports Hard Links
Supports Extended Attributes
Supports Open By FileID
Supports USN Journal

-- 
Yoshinao Muramatsu <ysno@ac.auone-net.jp>

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

Hi Yoshinao,

On Mar 18 14:29, Yoshinao Muramatsu wrote:
> Hi Corinna.
> I'm Yoshinao Muramatsu. Thank you for your thoughtful guidance.
> I have not modified the patch yet,
> but I have made some observations and post the results.
> 
> I forgot to mention in my first post that there is a workaround
> for this issue, which is to use process isolation unless
> hyper-v isolation is absolutely necessary
> (mainly caused by build number difference between host and container).

Ok, but that's a propoerty we'll never be able to test.

> In the logs, here we can find some differences.
> But I believe it is unclear if it will always be so.
> If additional inspections are done, they will be costly.

Assuming we can pull some information from the filesystem flags, the
cost is negligible.  We just check bits in a filesystem bitmask.

> I would also like to see better support for POSIX unlink/rename
> semantics in the container mechanism.

Yeah, but... do you have high hopes?

Assuming we can fetch useful info from the filesystem flags, we could
basically do both, add your patch to have a workaround in case the
Windows call returns INVALID_PARAMETER, and an early test for a FS
flag which can be used down the road to avoid the workaround.

> Therefore, I think it is not a bad idea to simply retry rather than spending
> time every time to check the conditions to deal with rare situations.
> However, in certain situations it happens all the time.
> (So if we can cache it that would be great.)
> But performance in the vast majority of other situations is more important.

The filesystem info is cached, so the filesystem flags are only checked
the first time we open a file on the filesystem.

> In the following logs, c:\ is the normal ntfs volume in the container,
> andc:\opt is the volume of the bind mounted host.
> Where the differences are
> - 0x00000020 Supports Disk Quotas
> - 0x01000000 Supports Open By FileID
> - 0x02000000 Supports USN Journal

I just updated the csih package to 0.9.13.  The getVolInfo tool
now prints *all* known filesystem flags.

> == hyper-v container (has issue)
> $ mount
> C:/cygwin64/bin on /usr/bin type ntfs (binary,auto)
> C:/cygwin64/lib on /usr/lib type ntfs (binary,auto)
> C:/cygwin64 on / type ntfs (binary,auto)
> C: on /cygdrive/c type ntfs (binary,posix=0,user,noumount,auto)
> 
> $ /usr/lib/csih/getVolInfo.exe /cygdrive/c/
> Device Type        : 7
> Characteristics    : 20020
> Volume Name        : <>
> Serial Number      : 3456656850
> Max Filenamelength : 255
> Filesystemname     : <NTFS>
> Flags              : 1c706df
> ...snip...
> 
> $ /usr/lib/csih/getVolInfo.exe /cygdrive/c/opt/
> Device Type        : 7
> Characteristics    : 20020
> Volume Name        : <>
> Serial Number      : 955187689
> Max Filenamelength : 255
> Filesystemname     : <NTFS>
> Flags              : 2c706ff
> ...snip...

This is disappointing.  One of the newer filesystem flags is
0x400, FILE_SUPPORTS_POSIX_UNLINK_RENAME.  As you can see,
the flag is set.  But the POSIX functionality doesn't work
here, right?

However, what's really curious here is the fact that the
FILE_SUPPORTS_OPEN_BY_FILE_ID flag is missing.

NTFS always supports this since Windows 2003!  So we could
use this flag as indicator that, probably, POSIX rename/unlink
won't work.


Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Yoshinao Muramatsu]

On 2023/03/18 19:01, Corinna Vinschen wrote:
>> In the logs, here we can find some differences.
>> But I believe it is unclear if it will always be so.
>> If additional inspections are done, they will be costly.
> 
> Assuming we can pull some information from the filesystem flags, the
> cost is negligible.  We just check bits in a filesystem bitmask.

I was expecting something like checking the file system driver
configuration as an additional test.
It would be ideal if the checks could be done at no additional cost.

> Assuming we can fetch useful info from the filesystem flags, we could
> basically do both, add your patch to have a workaround in case the
> Windows call returns INVALID_PARAMETER, and an early test for a FS
> flag which can be used down the road to avoid the workaround.

Adding workarounds can be done now and adding early testing
can be done once the judgment conditions are known. Great.

> This is disappointing.  One of the newer filesystem flags is
> 0x400, FILE_SUPPORTS_POSIX_UNLINK_RENAME.  As you can see,
> the flag is set.  But the POSIX functionality doesn't work
> here, right?

I was not aware that there was a FILE_SUPPORTS_POSIX_UNLINK_RENAME there.
I was only looking at the part where there was a difference.
Right, FILE_SUPPORTS_POSIX_UNLINK_RENAME doesn't work
on mounted volume with hyper-v isolation.

I additionally checked with the ltsc2016 version of the hyper-v container.
POSIX_UNLINK_RENAME should not be supported from the windows version,
but interesting results were obtained.
FILE_SUPPORTS_POSIX_UNLINK_RENAME is not considered to work,
but has not been tested.

> However, what's really curious here is the fact that the
> FILE_SUPPORTS_OPEN_BY_FILE_ID flag is missing.
> 
> NTFS always supports this since Windows 2003!  So we could
> use this flag as indicator that, probably, POSIX rename/unlink
> won't work.

I thought that if there was no FILE_SUPPORTS_OPEN_BY_FILE_ID
and OpenByFileId() worked, it would be a Signature,
but the result was different.

I tested OpenByFileID() on a bind-mounted directory, it was failed.
Maybe it's because of the path isolation.
ltsc2022 process isolation says "I support it" but it not works.
Okay, it's not a security issue. So now I'm writing this here.

Unfortunately, the state of the FILE_SUPPORTS_OPEN_BY_FILE_ID flag
does not match the actual behavior, so I fear it may be corrected.

FileSystemAttributes on containers
image   isolation mounted? FileSystemAttributes
ltsc2016 hyper-v  normal  0x1c700df        +OPEN_BYID
ltsc2022 hyper-v  normal  0x1c706df        +OPEN_BYID +POSIX +CLEANUP
ltsc2022 process  normal  0x1c706df        +OPEN_BYID +POSIX +CLEANUP
ltsc2016 hyper-v  binded  0x2c706ff +USN_J            +POSIX +CLEANUP +QUOTA
ltsc2022 hyper-v  binded  0x2c706ff +USN_J            +POSIX +CLEANUP +QUOTA
ltsc2022 process  binded  0x3e706ff +USN_J +OPEN_BYID +POSIX +CLEANUP +QUOTA

OpenByFileId and POSIX_UNLINK_RENAME test result
image    isolation mounted? OpenByFileId POSIX_UNLINK_RENAME
ltsc2016 hyper-v   normal   success      fail?(windows version)
ltsc2022 hyper-v   normal   success      success
ltsc2022 process   normal   success      success
ltsc2016 hyper-v   binded   fail         not tested
ltsc2022 hyper-v   binded   fail         fail
ltsc2022 process   binded   fail         success

ps
Corinna, I read the new email about fs_info::update patch
you posted while I was writing this.
I will report back when I test it, but it may take some time
as I usually use msys2 and don't have a cygwin development environment.

-- 
Yoshinao Muramatsu <ysno@ac.auone-net.jp>

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

On Mar 20 22:06, Yoshinao Muramatsu wrote:
> On 2023/03/18 19:01, Corinna Vinschen wrote:
> > FILE_SUPPORTS_OPEN_BY_FILE_ID flag is missing.
> > 
> > NTFS always supports this since Windows 2003!  So we could
> > use this flag as indicator that, probably, POSIX rename/unlink
> > won't work.
> 
> I thought that if there was no FILE_SUPPORTS_OPEN_BY_FILE_ID
> and OpenByFileId() worked, it would be a Signature,
> but the result was different.
> 
> I tested OpenByFileID() on a bind-mounted directory, it was failed.
> Maybe it's because of the path isolation.
> ltsc2022 process isolation says "I support it" but it not works.
> Okay, it's not a security issue. So now I'm writing this here.
> 
> Unfortunately, the state of the FILE_SUPPORTS_OPEN_BY_FILE_ID flag
> does not match the actual behavior, so I fear it may be corrected.

Actually, it doesn't matter if open by fileID works.  The fact that
this flag is missing *is* a pattern we can use.  It allows us to
distinguish the hyper-v isolated NTFS from a standard NTFS and thus,
we can immediately do the right thing.

> ps
> Corinna, I read the new email about fs_info::update patch
> you posted while I was writing this.
> I will report back when I test it, but it may take some time
> as I usually use msys2 and don't have a cygwin development environment.

No worries!


Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

On Mar 20 15:51, Corinna Vinschen wrote:
> On Mar 20 22:06, Yoshinao Muramatsu wrote:
> > On 2023/03/18 19:01, Corinna Vinschen wrote:
> > > FILE_SUPPORTS_OPEN_BY_FILE_ID flag is missing.
> > > 
> > > NTFS always supports this since Windows 2003!  So we could
> > > use this flag as indicator that, probably, POSIX rename/unlink
> > > won't work.
> > 
> > I thought that if there was no FILE_SUPPORTS_OPEN_BY_FILE_ID
> > and OpenByFileId() worked, it would be a Signature,
> > but the result was different.
> > 
> > I tested OpenByFileID() on a bind-mounted directory, it was failed.
> > Maybe it's because of the path isolation.
> > ltsc2022 process isolation says "I support it" but it not works.
> > Okay, it's not a security issue. So now I'm writing this here.
> > 
> > Unfortunately, the state of the FILE_SUPPORTS_OPEN_BY_FILE_ID flag
> > does not match the actual behavior, so I fear it may be corrected.
> 
> Actually, it doesn't matter if open by fileID works.  The fact that
> this flag is missing *is* a pattern we can use.  It allows us to
> distinguish the hyper-v isolated NTFS from a standard NTFS and thus,
> we can immediately do the right thing.

...and if Microsoft actually changes that at one point, your fallback
code will still work :)


Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

On Mar 20 15:51, Corinna Vinschen wrote:
> On Mar 20 22:06, Yoshinao Muramatsu wrote:
> > On 2023/03/18 19:01, Corinna Vinschen wrote:
> > > FILE_SUPPORTS_OPEN_BY_FILE_ID flag is missing.
> > > 
> > > NTFS always supports this since Windows 2003!  So we could
> > > use this flag as indicator that, probably, POSIX rename/unlink
> > > won't work.
> > 
> > I thought that if there was no FILE_SUPPORTS_OPEN_BY_FILE_ID
> > and OpenByFileId() worked, it would be a Signature,
> > but the result was different.
> > 
> > I tested OpenByFileID() on a bind-mounted directory, it was failed.
> > Maybe it's because of the path isolation.
> > ltsc2022 process isolation says "I support it" but it not works.
> > Okay, it's not a security issue. So now I'm writing this here.
> > 
> > Unfortunately, the state of the FILE_SUPPORTS_OPEN_BY_FILE_ID flag
> > does not match the actual behavior, so I fear it may be corrected.
> 
> Actually, it doesn't matter if open by fileID works.  The fact that
> this flag is missing *is* a pattern we can use.  It allows us to
> distinguish the hyper-v isolated NTFS from a standard NTFS and thus,
> we can immediately do the right thing.
> 
> > ps
> > Corinna, I read the new email about fs_info::update patch
> > you posted while I was writing this.
> > I will report back when I test it, but it may take some time
> > as I usually use msys2 and don't have a cygwin development environment.
> 
> No worries!

Wait.  I might have misunderstood something.  This is about accessing a
host NTFS from inside a Hyper-V isolated process, right?  So from the
point of view of the Hyper-V isolated Cygwin process, the NTFS
filesystem is a *local* filesystem?  Or is it mapped as a remote
filesystem?

The difference is important, because my patch would only change the
outcome if the Cygwin process in the Hyper-V container gets the
NTFS filesystem presented as a remote filesystem.

I noticed this problem when I was looking into implementing the
FILE_SUPPORTS_OPEN_BY_FILE_ID flag checking.  I have a different
solution from the one I pushed today in the loop which probably
makesmuch more sense and is independent of the subtil difference
between loca and remote FS.


Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Yoshinao Muramatsu]

> Wait.  I might have misunderstood something.  This is about accessing a
> host NTFS from inside a Hyper-V isolated process, right?  So from the
> point of view of the Hyper-V isolated Cygwin process, the NTFS
> filesystem is a *local* filesystem?  Or is it mapped as a remote
> filesystem?
> 
> The difference is important, because my patch would only change the
> outcome if the Cygwin process in the Hyper-V container gets the
> NTFS filesystem presented as a remote filesystem.
> 
> I noticed this problem when I was looking into implementing the
> FILE_SUPPORTS_OPEN_BY_FILE_ID flag checking.  I have a different
> solution from the one I pushed today in the loop which probably
> makesmuch more sense and is independent of the subtil difference
> between loca and remote FS.

I don't understand the whole picture, so I may be missing the point.
So I will report only the points I am sure of.

The file system on the host side is the local disk.
In the container, it appears to be exposed as a ntfs local
file system because unlink_nt() tries to use posix unlink semantics.

-- 
Yoshinao Muramatsu <ysno@ac.auone-net.jp>

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

On Mar 22 00:32, Yoshinao Muramatsu wrote:
> > Wait.  I might have misunderstood something.  This is about accessing a
> > host NTFS from inside a Hyper-V isolated process, right?  So from the
> > point of view of the Hyper-V isolated Cygwin process, the NTFS
> > filesystem is a *local* filesystem?  Or is it mapped as a remote
> > filesystem?
> > 
> > The difference is important, because my patch would only change the
> > outcome if the Cygwin process in the Hyper-V container gets the
> > NTFS filesystem presented as a remote filesystem.
> > 
> > I noticed this problem when I was looking into implementing the
> > FILE_SUPPORTS_OPEN_BY_FILE_ID flag checking.  I have a different
> > solution from the one I pushed today in the loop which probably
> > makesmuch more sense and is independent of the subtil difference
> > between loca and remote FS.
> 
> I don't understand the whole picture, so I may be missing the point.
> So I will report only the points I am sure of.
> 
> The file system on the host side is the local disk.
> In the container, it appears to be exposed as a ntfs local
> file system because unlink_nt() tries to use posix unlink semantics.

Right.  That and the fact that the filesystem characteristic don't have
the FILE_REMOTE_DEVICE flag set, as your getVolInfo output already
shows.  I could just have re-read your mail instead of asking redundant
questions, sorry!

I pushed a new Cygwin DLL, test release 3.5.0-0.251.gfe2545e9faaf.
This should do what we want, now.  If you can confirm, I'll push
your workaround afterwards.


Thanks,
Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Yoshinao Muramatsu]

On 2023/03/22 2:58, Corinna Vinschen wrote:
 > I pushed a new Cygwin DLL, test release 3.5.0-0.251.gfe2545e9faaf.
 > This should do what we want, now.  If you can confirm, I'll push
 > your workaround afterwards.

I have tested cygwin-3.5.0-0.251.gfe2545e9faaf.
It works fine with bind mounted directory in the hyper-v container.

However, the currently reported FILE_SUPPORTS_POSIX_UNLINK_RENAME
and FILE_SUPPORTS_OPEN_BY_FILE_ID are different from the
actual behavior and I will report this as a bug at
https://github.com/microsoft/Windows-Containers/issues/341

In the future, I expect that FILE_SUPPORTS_POSIX_UNLINK_RENAME will
be desirable as a check target, but then checking
FILE_SUPPORTS_OPEN_BY_FILE_ID in gfe2545e9faafd may become harmful.
(in process isolated container,
  posix unlink/rename works but OpenByFileId() not)

At this time, using FILE_SUPPORTS_POSIX_UNLINK_RENAME for
the check would mean the occurrence of additional failures
and subsequent workarounds.
This may be too optimistic, but STATUS_INVALID_PARAMETER is
an error at the parameter check stage, so I expect a small loss
in case of failure. Is the additional cost of unlink/rename failure
due to an incorrect FILE_SUPPORTS_POSIX_UNLINK_RENAME unacceptable?

----

Slightly off-topic, but since I could not use gui to set up cygwin
in the container, I am using setup-x86_64.exe with cli.
Is there a way to install the cygwin package by specifying
the package version from cli?
This time I modified setup.ini and installed with -l -L.

-- 
Yoshinao Muramatsu <ysno@ac.auone-net.jp>

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

On Mar 24 01:40, Yoshinao Muramatsu wrote:
> On 2023/03/22 2:58, Corinna Vinschen wrote:
> > I pushed a new Cygwin DLL, test release 3.5.0-0.251.gfe2545e9faaf.
> > This should do what we want, now.  If you can confirm, I'll push
> > your workaround afterwards.
> 
> I have tested cygwin-3.5.0-0.251.gfe2545e9faaf.
> It works fine with bind mounted directory in the hyper-v container.

Thanks, I pushed your patches.  We can reevaluate the
FILE_SUPPORTS_OPEN_BY_FILE_ID handling if Microsoft actually 
changes this in Hyper-V.

> Slightly off-topic, but since I could not use gui to set up cygwin
> in the container, I am using setup-x86_64.exe with cli.
> Is there a way to install the cygwin package by specifying
> the package version from cli?

Uhm... I don't think so.  I'm not aware of such a way to define the
desired package version on the CLI.  That would be a nice feature...


Thanks,
Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Jon Turney]

On 24/03/2023 11:54, Corinna Vinschen wrote:
> On Mar 24 01:40, Yoshinao Muramatsu wrote:
>> On 2023/03/22 2:58, Corinna Vinschen wrote:
>>> I pushed a new Cygwin DLL, test release 3.5.0-0.251.gfe2545e9faaf.
>>> This should do what we want, now.  If you can confirm, I'll push
>>> your workaround afterwards.
>>
>> I have tested cygwin-3.5.0-0.251.gfe2545e9faaf.
>> It works fine with bind mounted directory in the hyper-v container.
> 
> Thanks, I pushed your patches.  We can reevaluate the
> FILE_SUPPORTS_OPEN_BY_FILE_ID handling if Microsoft actually
> changes this in Hyper-V.
> 
>> Slightly off-topic, but since I could not use gui to set up cygwin
>> in the container, I am using setup-x86_64.exe with cli.
>> Is there a way to install the cygwin package by specifying
>> the package version from cli?
> 
> Uhm... I don't think so.  I'm not aware of such a way to define the
> desired package version on the CLI.  That would be a nice feature...

'-P cygwin=3.5.0-0.251.gfe2545e9faaf' allegedly works, for a while now.

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Corinna Vinschen]

On Mar 24 13:20, Jon Turney wrote:
> On 24/03/2023 11:54, Corinna Vinschen wrote:
> > On Mar 24 01:40, Yoshinao Muramatsu wrote:
> > > On 2023/03/22 2:58, Corinna Vinschen wrote:
> > > > I pushed a new Cygwin DLL, test release 3.5.0-0.251.gfe2545e9faaf.
> > > > This should do what we want, now.  If you can confirm, I'll push
> > > > your workaround afterwards.
> > > 
> > > I have tested cygwin-3.5.0-0.251.gfe2545e9faaf.
> > > It works fine with bind mounted directory in the hyper-v container.
> > 
> > Thanks, I pushed your patches.  We can reevaluate the
> > FILE_SUPPORTS_OPEN_BY_FILE_ID handling if Microsoft actually
> > changes this in Hyper-V.
> > 
> > > Slightly off-topic, but since I could not use gui to set up cygwin
> > > in the container, I am using setup-x86_64.exe with cli.
> > > Is there a way to install the cygwin package by specifying
> > > the package version from cli?
> > 
> > Uhm... I don't think so.  I'm not aware of such a way to define the
> > desired package version on the CLI.  That would be a nice feature...
> 
> '-P cygwin=3.5.0-0.251.gfe2545e9faaf' allegedly works, for a while now.

Oh, wow, great!


Thanks,
Corinna

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Jon Turney]

On 24/03/2023 13:22, Corinna Vinschen wrote:
> On Mar 24 13:20, Jon Turney wrote:
>> On 24/03/2023 11:54, Corinna Vinschen wrote:
>>> On Mar 24 01:40, Yoshinao Muramatsu wrote:
>>>> On 2023/03/22 2:58, Corinna Vinschen wrote:
>>>>> I pushed a new Cygwin DLL, test release 3.5.0-0.251.gfe2545e9faaf.
>>>>> This should do what we want, now.  If you can confirm, I'll push
>>>>> your workaround afterwards.
>>>>
>>>> I have tested cygwin-3.5.0-0.251.gfe2545e9faaf.
>>>> It works fine with bind mounted directory in the hyper-v container.
>>>
>>> Thanks, I pushed your patches.  We can reevaluate the
>>> FILE_SUPPORTS_OPEN_BY_FILE_ID handling if Microsoft actually
>>> changes this in Hyper-V.
>>>
>>>> Slightly off-topic, but since I could not use gui to set up cygwin
>>>> in the container, I am using setup-x86_64.exe with cli.
>>>> Is there a way to install the cygwin package by specifying
>>>> the package version from cli?
>>>
>>> Uhm... I don't think so.  I'm not aware of such a way to define the
>>> desired package version on the CLI.  That would be a nice feature...
>>
>> '-P cygwin=3.5.0-0.251.gfe2545e9faaf' allegedly works, for a while now.
> 
> Oh, wow, great!

In this particular instance, just using '-t' (equivalent to the 'enable 
test packages') checkbox, might be less typing and just as good.

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container

[Yoshinao Muramatsu]

On 2023/03/24 20:54, Corinna Vinschen wrote:
 > Thanks, I pushed your patches.  We can reevaluate the
 > FILE_SUPPORTS_OPEN_BY_FILE_ID handling if Microsoft actually
 > changes this in Hyper-V.

Great! Now we've got a working implementation and
we know why the code is the way it is.

I use msys2 and have found via this issue that
the cygwin code achieves a lot.
Many thanks for all the contributions
and efforts made thus far.

-- 
Yoshinao Muramatsu <ysno@ac.auone-net.jp>

[PATCH 0/3] fix unlink/rename failure in hyper-v container(regenerate)

[Yoshinao Muramatsu]

From: Yoshinao <ysno@ac.auone-net.jp>

use real name and add `Signed-Off-By:' field, as suggested by Corinna.
code is untouched.

Yoshinao Muramatsu (3):
  fix unlink in container
  fix rename in container
  log disabling posix semantics

 winsup/cygwin/syscalls.cc | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

-- 
2.37.3.windows.1

[PATCH 1/3] fix unlink in container

[Yoshinao Muramatsu]

Deleting files returns STATUS_INVALID_PARAMETE on a bind mountedfile system
in hyper-v container with FILE_DISPOSITION_POSIX_SEMANTICS.
Therefore fall back to default method.

This code is suggeted by Johannes Schindelin on github
and I change it more simple.

Signed-off-by: Yoshinao Muramatsu <ysno@ac.auone-net.jp>
---
 winsup/cygwin/syscalls.cc | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 8ae0397fb..7430fad65 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -731,7 +731,10 @@ _unlink_nt (path_conv &pc, bool shareable)
       /* Trying to delete in-use executables and DLLs using
          FILE_DISPOSITION_POSIX_SEMANTICS returns STATUS_CANNOT_DELETE.
 	 Fall back to the default method. */
-      if (status != STATUS_CANNOT_DELETE)
+      /* Additionaly that returns STATUS_INVALID_PARAMETER
+         on a bind mounted fs in hyper-v container. Falling back too. */
+      if (status != STATUS_CANNOT_DELETE
+          && status != STATUS_INVALID_PARAMETER)
 	goto out;
     }
 
-- 
2.37.3.windows.1

[PATCH 2/3] fix rename in container

[Yoshinao Muramatsu]

Renaming files returns STATUS_INVALID_PARAMETE on a bind mounted file system
in hyper-v container with FILE_RENAME_POSIX_SEMANTICS.

Disable the use_posix_semantics flag and retry.

Signed-off-by: Yoshinao Muramatsu <ysno@ac.auone-net.jp>
---
 winsup/cygwin/syscalls.cc | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 7430fad65..57bfab9d3 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -2427,6 +2427,7 @@ rename2 (const char *oldpath, const char *newpath, unsigned int at2flags)
 			    && !oldpc.isremote ()
 			    && oldpc.fs_is_ntfs ();
 
+ignore_posix_semantics_retry:
       /* Opening the file must be part of the transaction.  It's not sufficient
 	 to call only NtSetInformationFile under the transaction.  Therefore we
 	 have to start the transaction here, if necessary.  Don't start
@@ -2671,6 +2672,15 @@ skip_pre_W10_checks:
 	    unlink_nt (*removepc);
 	  res = 0;
 	}
+      else if (use_posix_semantics && status == STATUS_INVALID_PARAMETER)
+        {
+          /* NtSetInformationFile returns STATUS_INVALID_PARAMETER
+             on a bind mounted file system in hyper-v container
+             with FILE_RENAME_POSIX_SEMANTICS.
+             Disable the use_posix semntics flag and retry. */
+          use_posix_semantics = 0;
+          goto ignore_posix_semantics_retry;
+        }
       else
 	__seterrno_from_nt_status (status);
     }
-- 
2.37.3.windows.1

[PATCH 3/3] log disabling posix semantics

[Yoshinao Muramatsu]

Add log when workaround occurs

Signed-off-by: Yoshinao Muramatsu <ysno@ac.auone-net.jp>
---
 winsup/cygwin/syscalls.cc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 57bfab9d3..bc93c16d8 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -735,7 +735,11 @@ _unlink_nt (path_conv &pc, bool shareable)
          on a bind mounted fs in hyper-v container. Falling back too. */
       if (status != STATUS_CANNOT_DELETE
           && status != STATUS_INVALID_PARAMETER)
-	goto out;
+        {
+          debug_printf ("NtSetInformationFile returns %y "
+                        "with posix semantics. Disable it and retry.", status);
+          goto out;
+        }
     }
 
   /* If the R/O attribute is set, we have to open the file with
@@ -2678,6 +2682,8 @@ skip_pre_W10_checks:
              on a bind mounted file system in hyper-v container
              with FILE_RENAME_POSIX_SEMANTICS.
              Disable the use_posix semntics flag and retry. */
+          debug_printf ("NtSetInformationFile failed with posix semantics. "
+                        "Disable it and retry.");
           use_posix_semantics = 0;
           goto ignore_posix_semantics_retry;
         }
-- 
2.37.3.windows.1

Re: [PATCH 0/3] fix unlink/rename failure in hyper-v container(regenerate)

[Corinna Vinschen]

Hi Yoshinao,

I just pushed a patch to fs_info::update to take the filesystem
flags introduced starting with Windows 7 into account:

https://sourceware.org/cgit/newlib-cygwin/commit/?id=fcccdc4021ff

AFAICS, this patch should already fix the problem.

It's not the last state, though, because a few more tweak are necessary:

- The filesystem shows up as "cifs" rather than "ntfs" in mount.
- Creating case sensitive directories won't work.
- an upper/lowercase problem in user extended attributes.
- unlinking files-in-use is broken yet.

I intend to fix those in a followup patch.

The next cygwin test release 3.5.0-0.249.g59241913330c will contain the
above patch.  Before we go ahead and apply your patch, can you test if
this already works as desired?  I'd like to get this working and then we
add your workaround, in case we encounter this problem in another
scenario, too.


Thanks,
Corinna



On Mar 20 20:50, Yoshinao Muramatsu wrote:
> From: Yoshinao <ysno@ac.auone-net.jp>
> 
> use real name and add `Signed-Off-By:' field, as suggested by Corinna.
> code is untouched.
> 
> Yoshinao Muramatsu (3):
>   fix unlink in container
>   fix rename in container
>   log disabling posix semantics
> 
>  winsup/cygwin/syscalls.cc | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> -- 
> 2.37.3.windows.1