chmod failure on GVFS mounted CIFS share

chmod failure on GVFS mounted CIFS share

[Konrad Bucheli (PSI)]

Dear git developers

Below my bug report:

What did you do before the bug happened? (Steps to reproduce your issue)

I do a `git init .` on a directory which is CIFS mounted via gio mount 
(FUSE).
On RHEL8 this needs gvfs-smb and gvfs-fuse installed.
Mount command: gio mount smb://fs01.psi.ch/my_user_name$

What did you expect to happen? (Expected behavior)

It initializes the git repo.

What happened instead? (Actual behavior)

$ git init .
error: chmod on 
/run/user/44951/gvfs/smb-share:server=fs01.psi.ch,share=my_user_name$/git/foo/.git/config.lock 
failed: Operation not supported
fatal: could not set 'core.filemode' to 'false'
$

What's different between what you expected and what actually happened?

I expect that it fully initializes the repository:

[buchel_k@lxdev07 foo]$ git status
fatal: not a git repository (or any parent up to mount point 
/run/user/44951)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
$ find .
.
./.git
./.git/branches
./.git/hooks
./.git/hooks/applypatch-msg.sample
./.git/hooks/commit-msg.sample
./.git/hooks/post-update.sample
./.git/hooks/pre-applypatch.sample
./.git/hooks/pre-commit.sample
./.git/hooks/pre-merge-commit.sample
./.git/hooks/pre-push.sample
./.git/hooks/pre-receive.sample
./.git/hooks/push-to-checkout.sample
./.git/hooks/sendemail-validate.sample
./.git/hooks/update.sample
./.git/hooks/fsmonitor-watchman.sample
./.git/hooks/pre-rebase.sample
./.git/hooks/prepare-commit-msg.sample
./.git/info
./.git/info/exclude
./.git/description
./.git/refs
./.git/refs/heads
./.git/refs/tags
./.git/HEAD
./.git/config
./git-bugreport-2024-12-12-0951.txt
$


Anything else you want to add:

I guess the failure happens at config.c:3339 
(https://github.com/git/git/blob/master/config.c#L3339).
What would it mean if the chmod there ignores "Operation not supported"?

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.43.5
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 4.18.0-553.30.1.el8_10.x86_64 #1 SMP Fri Nov 15 03:46:25 
EST 2024 x86_64
compiler info: gnuc: 8.5
libc info: glibc: 2.28
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]
not run from a git repository - no hooks to show


Thank you very much for your good work.

Kind regards

Konrad

-- 
Paul Scherrer Institut
Konrad Bucheli
Linux Systems Engineer
Core Linux Research Services
Science IT Infrastructure and Services department (AWI)
OBBA/230
Forschungstrasse 111
5232 Villigen PSI
Switzerland

Phone: +41 56 310 27 24
konrad.bucheli@psi.ch
www.psi.ch

Re: chmod failure on GVFS mounted CIFS share

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

On 2024-12-12 at 09:14:50, Konrad Bucheli (PSI) wrote:
> Dear git developers
> 
> Below my bug report:
> 
> What did you do before the bug happened? (Steps to reproduce your issue)
> 
> I do a `git init .` on a directory which is CIFS mounted via gio mount
> (FUSE).
> On RHEL8 this needs gvfs-smb and gvfs-fuse installed.
> Mount command: gio mount smb://fs01.psi.ch/my_user_name$

In general, gvfs's FUSE driver isn't a good way to interact with files.
My experience with its SFTP driver is that it has a bunch of weird,
non-Unixy behaviour that's due to limitations in the gio interface.  So
it probably doesn't provide the functionality most Unix programs will
expect from a file system, which will cause you a world of problems down
the line, as you've seen here.

> What did you expect to happen? (Expected behavior)
> 
> It initializes the git repo.
> 
> What happened instead? (Actual behavior)
> 
> $ git init .
> error: chmod on /run/user/44951/gvfs/smb-share:server=fs01.psi.ch,share=my_user_name$/git/foo/.git/config.lock
> failed: Operation not supported
> fatal: could not set 'core.filemode' to 'false'
> $

This is indeed the case, since the chmod on the config file fails.  We
always rewrite the config file as a separate lock file, and then rename
into place.  (This prevents concurrent modification correctly even on
network file systems.)  The user generally wants the permissions to be
preserved, so this is the safe default.

This also happens when using a Linux Git on a WSL Windows mount.

Note that even Windows honours the read-only/read-write difference for
files, so in theory chmod is useful even on CIFS and other Windows file
systems.

If I remember correctly, the consensus last time this came up was that
someone is welcome to add a config option that ignores the chmod
failure, but that in general, we don't want to just silently ignore it.
Unfortunately, nobody has added such a configuration option yet.

I will note that the `mount.cifs(8)` manual page[0] indicates that the
Linux kernel CIFS driver _does_ silently ignore chmod operations when
the server doesn't support them, but it also supports using and storing
Unix permissions if the server does, so that may be a workaround.

[0] https://linux.die.net/man/8/mount.cifs
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: chmod failure on GVFS mounted CIFS share

[Konrad Bucheli (PSI)]

On 13.12.24 04:41, brian m. carlson wrote:
> On 2024-12-12 at 09:14:50, Konrad Bucheli (PSI) wrote:
>> Dear git developers
>>
>> Below my bug report:
>>
>> What did you do before the bug happened? (Steps to reproduce your issue)
>>
>> I do a `git init .` on a directory which is CIFS mounted via gio mount
>> (FUSE).
>> On RHEL8 this needs gvfs-smb and gvfs-fuse installed.
>> Mount command: gio mount smb://fs01.psi.ch/my_user_name$
> 
> In general, gvfs's FUSE driver isn't a good way to interact with files.
> My experience with its SFTP driver is that it has a bunch of weird,
> non-Unixy behaviour that's due to limitations in the gio interface.  So
> it probably doesn't provide the functionality most Unix programs will
> expect from a file system, which will cause you a world of problems down
> the line, as you've seen here.
> 
True, but it has a big advantage: you can mount it without being root.
>> What did you expect to happen? (Expected behavior)
>>
>> It initializes the git repo.
>>
>> What happened instead? (Actual behavior)
>>
>> $ git init .
>> error: chmod on /run/user/44951/gvfs/smb-share:server=fs01.psi.ch,share=my_user_name$/git/foo/.git/config.lock
>> failed: Operation not supported
>> fatal: could not set 'core.filemode' to 'false'
>> $
> 
> This is indeed the case, since the chmod on the config file fails.  We
> always rewrite the config file as a separate lock file, and then rename
> into place.  (This prevents concurrent modification correctly even on
> network file systems.)  The user generally wants the permissions to be
> preserved, so this is the safe default.
> 
> This also happens when using a Linux Git on a WSL Windows mount.
> 
> Note that even Windows honours the read-only/read-write difference for
> files, so in theory chmod is useful even on CIFS and other Windows file
> systems.
> 
> If I remember correctly, the consensus last time this came up was that
> someone is welcome to add a config option that ignores the chmod
> failure, but that in general, we don't want to just silently ignore it.
> Unfortunately, nobody has added such a configuration option yet.
My suggestion is not to silently ignore all chmod errors, only ENOTSUP 
for config.lock which basically tells that it is not a suitable location 
for that operation and thus also not required.
Would that be acceptable?

Re: chmod failure on GVFS mounted CIFS share

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On 2024-12-13 at 10:32:22, Konrad Bucheli (PSI) wrote:
> My suggestion is not to silently ignore all chmod errors, only ENOTSUP for
> config.lock which basically tells that it is not a suitable location for
> that operation and thus also not required.
> Would that be acceptable?

That solves your particular problem, but the WSL situation gets
EPERM[0], so it would not solve the general problem.  We definitely
don't want to blindly ignore EPERM in general for `config.lock` because
it could indicate a real permissions problem or a race condition with
other software.

[0] https://askubuntu.com/questions/1115564/wsl-ubuntu-distro-how-to-solve-operation-not-permitted-on-cloning-repository
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: chmod failure on GVFS mounted CIFS share

[Konrad Bucheli (PSI)]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

I have another idea: there is no need for a chmod if both the config 
file and the lock file already have he same mode. Which is the case if 
the filesystem has no proper chmod support.

Attached patch implements this and I could successfully initialize a new 
git repo on a CIFS share accessed over FUSE.

What do you think?

On 15.12.24 23:30, brian m. carlson wrote:
> On 2024-12-13 at 10:32:22, Konrad Bucheli (PSI) wrote:
>> My suggestion is not to silently ignore all chmod errors, only ENOTSUP for
>> config.lock which basically tells that it is not a suitable location for
>> that operation and thus also not required.
>> Would that be acceptable?
> 
> That solves your particular problem, but the WSL situation gets
> EPERM[0], so it would not solve the general problem.  We definitely
> don't want to blindly ignore EPERM in general for `config.lock` because
> it could indicate a real permissions problem or a race condition with
> other software.
> 
> [0] https://askubuntu.com/questions/1115564/wsl-ubuntu-distro-how-to-solve-operation-not-permitted-on-cloning-repository

[-- Attachment #2: config_with_less_chmod.patch --]
[-- Type: text/x-patch, Size: 1207 bytes --]

diff --git a/config.c b/config.c
index a11bb85da3..3466f2ca4c 100644
--- a/config.c
+++ b/config.c
@@ -3256,7 +3256,7 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 		    write_pair(fd, key, value, comment, &store) < 0)
 			goto write_err_out;
 	} else {
-		struct stat st;
+		struct stat st, st_lock;
 		size_t copy_begin, copy_end;
 		int i, new_line = 0;
 		struct config_options opts;
@@ -3336,12 +3336,20 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 		close(in_fd);
 		in_fd = -1;
 
-		if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
-			error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
+		if (stat(get_lock_file_path(&lock), &st_lock) == -1) {
+			error_errno(_("stat on %s failed"), get_lock_file_path(&lock));
 			ret = CONFIG_NO_WRITE;
 			goto out_free;
 		}
 
+		if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {
+			if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
+				error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
+				ret = CONFIG_NO_WRITE;
+				goto out_free;
+			}
+		}
+
 		if (store.seen_nr == 0) {
 			if (!store.seen_alloc) {
 				/* Did not see key nor section */

Re: chmod failure on GVFS mounted CIFS share

[Junio C Hamano]

"Konrad Bucheli (PSI)" <konrad.bucheli@psi.ch> writes:

> I have another idea: there is no need for a chmod if both the config
> file and the lock file already have he same mode. Which is the case if
> the filesystem has no proper chmod support.

And for majority of people who have working chmod(), would it mean
one extra and unnecessary system call?

Instead, how about doing the chmod() first, like we have always done,
but after seeing it fail, check with lstat() to see if the modes are
already in the desired state and refrain from complaining if that is
the case?  That way, we'll incur extra overhead only in the error
code path, which is the usual pattern we would prefer to do things.

So instead of removing this part, ...

> -		if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
> -			error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));

... you'd do an extra lstat() on the lock file (so you can move the
st_lock inside this block, narrowing its scope) before calling
error_errno(), and only after finding out that st_lock cannot
somehow be obtained or the resulting mode is different, you call
error_errno() and arrange an error to be returned, all inside the
if(){} block of the original.

Wouldn't it work even better, I wonder?

Thanks.

> +		if (stat(get_lock_file_path(&lock), &st_lock) == -1) {
> +			error_errno(_("stat on %s failed"), get_lock_file_path(&lock));
>  			ret = CONFIG_NO_WRITE;
>  			goto out_free;
>  		}
>  
> +		if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {
> +			if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
> +				error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
> +				ret = CONFIG_NO_WRITE;
> +				goto out_free;
> +			}
> +		}
> +
>  		if (store.seen_nr == 0) {
>  			if (!store.seen_alloc) {
>  				/* Did not see key nor section */

Re: chmod failure on GVFS mounted CIFS share

[Konrad Bucheli (PSI)]

On 20.12.24 16:44, Junio C Hamano wrote:
> "Konrad Bucheli (PSI)" <konrad.bucheli@psi.ch> writes:
> 
>> I have another idea: there is no need for a chmod if both the config
>> file and the lock file already have he same mode. Which is the case if
>> the filesystem has no proper chmod support.
> 
> And for majority of people who have working chmod(), would it mean
> one extra and unnecessary system call?
> 

I do not have stats, but I guess the chmod call would be needed very 
rarely as most of the time both files have default permissions. I do not 
know which call is more expensive.

> Instead, how about doing the chmod() first, like we have always done,
> but after seeing it fail, check with lstat() to see if the modes are
> already in the desired state and refrain from complaining if that is
> the case?  That way, we'll incur extra overhead only in the error
> code path, which is the usual pattern we would prefer to do things.
> 
> So instead of removing this part, ...
> 
>> -		if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
>> -			error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
> 
> ... you'd do an extra lstat() on the lock file (so you can move the
> st_lock inside this block, narrowing its scope) before calling
> error_errno(), and only after finding out that st_lock cannot
> somehow be obtained or the resulting mode is different, you call
> error_errno() and arrange an error to be returned, all inside the
> if(){} block of the original.
> 
> Wouldn't it work even better, I wonder?

If you think that is the way to go, I will adapt the patch.

> 
> Thanks.
> 
>> +		if (stat(get_lock_file_path(&lock), &st_lock) == -1) {
>> +			error_errno(_("stat on %s failed"), get_lock_file_path(&lock));
>>   			ret = CONFIG_NO_WRITE;
>>   			goto out_free;
>>   		}
>>   
>> +		if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {
>> +			if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
>> +				error_errno(_("chmod on %s failed"), get_lock_file_path(&lock));
>> +				ret = CONFIG_NO_WRITE;
>> +				goto out_free;
>> +			}
>> +		}
>> +
>>   		if (store.seen_nr == 0) {
>>   			if (!store.seen_alloc) {
>>   				/* Did not see key nor section */
> 

Re: chmod failure on GVFS mounted CIFS share

[Konrad Bucheli (PSI)]

Hi

I investigated some more git init failures on our NetApp backed CIFS 
storage, this time not with FUSE but a real kernel mount. I found two 
different situations where a seemingly working "no-op chmod" makes git 
init fail.

a) Everyone Full Control permissions on the backend NTFS

$ touch test.txt
$ ll
total 1
-------rwx 1 buchel_k root 0 10. Jan 10:28 test.txt
$ chmod 007 test.txt
$ ll
total 1
-------rwx 1 buchel_k root 0 10. Jan 10:28 test.txt
$ echo foo > test.txt
-bash: test.txt: Permission denied
$ rm test.txt
rm: cannot remove 'test.txt': Permission denied
$ chmod 600 test.txt
$ rm test.txt
$

Investigation in the backend filesystem showed that this created a "User 
Deny" rule which is stronger that the "Everyone Full Control" rule.

b) only User Write permissions on backend NTFS

This second issue is even more wired:

$ touch test.txt
$ ll
total 1
-rwx------ 1 buchel_k root 0 10. Jan 11:00 test.txt
$ # open the file and keep it open
$ exec {fd}>test.txt
$ chmod 700 test.txt
$ # this chmod took multiple seconds
$ ll
total 0
-rwx------ 1 buchel_k root 0 10. Jan 11:01 test.txt
$ # write to file using the already open filehandle
$ echo test >&$fd
-bash: echo: write error: Permission denied
$ # close filehandle
$ exec {fd}>&-
$ # repeat without chmod
$ exec {fd}>test.txt
$ echo test >&$fd
$ exec {fd}>&-
$ cat test.txt
test
$

So if the chmod to a file happens after is has been already opened, then 
subsequent write fail. Looks like a bug to me. After reopening, all is fine.

For the records:
mount options as listed by mount:
(rw,relatime,vers=3.1.1,sec=krb5,cruid=0,cache=strict,multiuser,uid=0,noforceuid,gid=0,noforcegid,addr=<REDACTED>,file_mode=0755,dir_mode=0755,soft,nounix,serverino,mapposix,cifsacl,noperm,reparse=nfs,rsize=1048576,wsize=1048576,bsize=1048576,retrans=1,echo_interval=60,actimeo=1,closetimeo=1)
Kernel: 5.14.0-503.19.1.el9_5.x86_64 (RHEL 9.5)
Backend: NetApp FAS8700 with Release 9.13.1P6


My conclusion is that a no-op chmod is fine on a local, proper UNIX 
filesystem, but in other setups it can have unexpected side effects or 
even hit bugs. So stat before chmod will make it work in more 
circumstances and cases. And as git is nowadays a ubiquitous tool, my 
users will use git wherever they believe it would be useful. I would 
like to give them a seamless experience as possible.

My experience might be limited, but I have not seen in my career any 
attempt or use case where permission bits inside .git where adjusted 
after. And only then there would be minimal price to pay with the extra 
stat, all other save a chmod. And this price is way smaller than having 
git init/clone fail completely.


What do you think?

Kind regards

Konrad


On 20.12.24 16:50, Konrad Bucheli (PSI) wrote:
> 
> 
> On 20.12.24 16:44, Junio C Hamano wrote:
>> "Konrad Bucheli (PSI)" <konrad.bucheli@psi.ch> writes:
>>
>>> I have another idea: there is no need for a chmod if both the config
>>> file and the lock file already have he same mode. Which is the case if
>>> the filesystem has no proper chmod support.
>>
>> And for majority of people who have working chmod(), would it mean
>> one extra and unnecessary system call?
>>
> 
> I do not have stats, but I guess the chmod call would be needed very 
> rarely as most of the time both files have default permissions. I do not 
> know which call is more expensive.
> 
>> Instead, how about doing the chmod() first, like we have always done,
>> but after seeing it fail, check with lstat() to see if the modes are
>> already in the desired state and refrain from complaining if that is
>> the case?  That way, we'll incur extra overhead only in the error
>> code path, which is the usual pattern we would prefer to do things.
>>
>> So instead of removing this part, ...
>>
>>> -        if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) < 0) {
>>> -            error_errno(_("chmod on %s failed"), 
>>> get_lock_file_path(&lock));
>>
>> ... you'd do an extra lstat() on the lock file (so you can move the
>> st_lock inside this block, narrowing its scope) before calling
>> error_errno(), and only after finding out that st_lock cannot
>> somehow be obtained or the resulting mode is different, you call
>> error_errno() and arrange an error to be returned, all inside the
>> if(){} block of the original.
>>
>> Wouldn't it work even better, I wonder?
> 
> If you think that is the way to go, I will adapt the patch.
> 
>>
>> Thanks.
>>
>>> +        if (stat(get_lock_file_path(&lock), &st_lock) == -1) {
>>> +            error_errno(_("stat on %s failed"), 
>>> get_lock_file_path(&lock));
>>>               ret = CONFIG_NO_WRITE;
>>>               goto out_free;
>>>           }
>>> +        if ((st.st_mode & 07777) != (st_lock.st_mode & 07777)) {
>>> +            if (chmod(get_lock_file_path(&lock), st.st_mode & 07777) 
>>> < 0) {
>>> +                error_errno(_("chmod on %s failed"), 
>>> get_lock_file_path(&lock));
>>> +                ret = CONFIG_NO_WRITE;
>>> +                goto out_free;
>>> +            }
>>> +        }
>>> +
>>>           if (store.seen_nr == 0) {
>>>               if (!store.seen_alloc) {
>>>                   /* Did not see key nor section */
>>
>