Bug 66251 - Git doesn't work correctly on samba share after upgrading kernel from 3.11.6 to 3.12.1
Summary: Git doesn't work correctly on samba share after upgrading kernel from 3.11.6 ...
Status: RESOLVED CODE_FIX
Alias: None
Product: File System
Classification: Unclassified
Component: CIFS (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: fs_cifs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-30 23:12 UTC by Maciej
Modified: 2019-02-08 14:36 UTC (History)
6 users (show)

See Also:
Kernel Version: 3.12.1
Tree: Mainline
Regression: Yes


Attachments
Set FILE_CREATED on O_CREAT|O_EXCL (2.38 KB, patch)
2013-12-11 22:34 UTC, Shirish Pargaonkar
Details | Diff

Description Maciej 2013-11-30 23:12:47 UTC
Description:

After upgrading kernel to 3.12.1 I cannot use 'git add' nor 'git commit' as regular user on samba share. The same commands work with sudo.


Steps to reproduce:

1. cd into already mounted share
2. mkdir test && cd test
3. git init
4. touch test
5. git add test


Additional info:

git 1.8.4.2, samba 4.1.1 on 64-bit Archlinux. I haven't been changing anything in git nor fstab configuration since ages. 
I've tried downgrading all other packages first to make sure it wasn't their fault - it hadn't helped. After downgrading kernel to 3.11.6 (last official 3.11.x kernel in Arch) the issue disappeared.

Share is mounted using systemd automount in /etc/fstab:

//SERVER/DATA /media/smb cifs user,noauto,credentials=/etc/samba/creds,\
workgroup=PRV,uid=1000,gid=users,_netdev,comment=systemd.automount 0 0

mount appears as:

//SERVER/DATA on /media/smb type cifs (rw,nosuid,nodev,noexec,relatime,vers=1.0,\
cache=strict,domain=PRV,uid=1000,forceuid,gid=100,forcegid,addr=10.1.1.5,\
file_mode=0755,dir_mode=0755,nounix,serverino,rsize=61440,wsize=65536,actimeo=1)

'git init && touch test && strace git add test' on empty directory inside the share returns:

open(".git/objects/info/alternates", O_RDONLY|O_NOATIME) = -1 ENOENT (No such file or directory)
access(".git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391", F_OK) = -1 ENOENT (No such file or directory)
open(".git/objects/e6/tmp_obj_GvIyn7", O_RDWR|O_CREAT|O_EXCL, 0444) = -1 EACCES (Permission denied)
write(2, "error: insufficient permission f"..., 88error: insufficient permission for adding an object to repository database .git/objects
) = 88
close(4) = 0
write(2, "error: test: failed to insert in"..., 44error: test: failed to insert into database
) = 44
write(2, "error: unable to index file test"..., 33error: unable to index file test
) = 33
Comment 1 Shirish Pargaonkar 2013-12-03 03:04:54 UTC
yes, I see the error.  Investigating.

git add test
error: insufficient permission for adding an object to repository database .git/objects
error: test: failed to insert into database
error: unable to index file test
fatal: adding files failed
Comment 2 Shirish Pargaonkar 2013-12-03 16:17:11 UTC
It does not make sense for git to open/create a file for read-write while 
specifying a mode of 444!

open(".git/objects/e6/tmp_obj_GvIyn7", O_RDWR|O_CREAT|O_EXCL, 0444)
Comment 3 Alan 2013-12-03 16:24:13 UTC
Actually it does.

http://pubs.opengroup.org/onlinepubs/007908775/xsh/open.html

the behaviour is defined by SuS, correct and valid

The question is why it breaks from 3.11 to 3.12 with SMB
Comment 4 Shirish Pargaonkar 2013-12-03 17:36:06 UTC
I do not see any error as far as smb traffic is concerned.
File does get created on the server with the intended permission.
Something to do with vfs / inode permissions etc. for open to fail
with EACCES, perhaps! Looking...
Comment 5 Shirish Pargaonkar 2013-12-05 05:17:21 UTC
For a mode of 0x26 (MAY_READ | MAY_WRITE | MAY_OPEN), generic_permission
returns -EACCES.  Not yet sure why...
Comment 6 Shirish Pargaonkar 2013-12-06 06:14:47 UTC
Need to understand what this line is saying in acl_permission_check()

if ((mask & ~mode & (MAY_READ | MAY_WRITE | MAY_EXEC)) == 0)
               return 0;

Instead of returning 0, we return -EACCES.
Comment 7 Shirish Pargaonkar 2013-12-07 04:14:05 UTC
commit 6d125529c6cbfe570ce3bf9a0728548f087499da brings in change to
macro ACC_MODE which changes acc_mode value calculated during
build_open_flags which fails inode_permission.

Need to understand what this change means and how/what to alter for a fix.

-#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
+#define ACC_MODE(x) ("\004\002\006\006"[(x)&O_ACCMODE])
Comment 8 Shirish Pargaonkar 2013-12-07 05:45:52 UTC
Need to look into mode (i_mode) value of the newly created inode as well...
Comment 9 Shirish Pargaonkar 2013-12-09 03:29:56 UTC
Need to understand the acc_mode/mask and inode Permission/mode interaction.
acl_permission_check() for a regular as well as superuser fails with new
acc_mode / maks in 3.12 kernel (0x26) which used to be (0x20) in 3.11.
But for superuser, inode_capable is true but false for a regular user.
Permission of 0x124 / 0x8124 / 0x204, not sure how all this works out.
Comment 10 Shirish Pargaonkar 2013-12-11 17:03:29 UTC
I think cifs needs to make changes related to commit 116cc0225381415b96551f725455d067f63a76a0

Working on it.
Comment 11 Shirish Pargaonkar 2013-12-11 22:34:11 UTC
Created attachment 118111 [details]
Set FILE_CREATED on O_CREAT|O_EXCL

Sent this patch to the cifs mailing list.  I tested it against a samba server
and seems to work.  Feedback appreciated.
Comment 12 Maciej 2013-12-12 06:54:02 UTC
I'm compiling kernel with the patch applied. I'll let you know when I'm done with testing.
Comment 13 Maciej 2013-12-12 09:14:21 UTC
Yes, it seems the problem is gone with your patch applied.
Comment 14 Maciej 2013-12-13 07:16:34 UTC
I've read messages for 116cc0225381415b96551f725455d067f63a76a0 and 01c919abaf2f3d6a8e59eddf4ee22df1631ab067 

It looks like the changes were prepared primarily for NFS with notice of "other vfs should do so too" sort.
I don't see any changes in CIFS regarding FILE_CREATED - maybe it just went unnoticed for other vfs' maintainers?
Comment 15 Shirish Pargaonkar 2013-12-13 14:42:57 UTC
Yes, I think so, that is what happened.
Comment 16 Ronnie Sahlberg 2019-02-07 23:09:09 UTC
This has been fixed upstream a long time ago so we should close this bug

commit f1e3268126a35b9d3cb8bf67487fcc6cd13991d8
Author: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
Date:   Wed Dec 11 16:29:53 2013 -0600

    cifs: set FILE_CREATED
    
    Set FILE_CREATED on O_CREAT|O_EXCL.
    
    cifs code didn't change during commit 116cc0225381415b96551f725455d067f63a76a0
    
    Kernel bugzilla 66251
    
    Signed-off-by: Shirish Pargaonkar <spargaonkar@suse.com>
    Acked-by: Jeff Layton <jlayton@redhat.com>
    CC: Stable <stable@kernel.org>
    Signed-off-by: Steve French <smfrench@gmail.com>
Comment 17 Ronnie Sahlberg 2019-02-07 23:10:57 UTC
Verified the fix works in current upstream:

[sahlberg@rawhide-2 mnt]$ mkdir git
[sahlberg@rawhide-2 mnt]$ cd git
[sahlberg@rawhide-2 git]$ git init
Initialized empty Git repository in /mnt/git/.git/
[sahlberg@rawhide-2 git]$ touch test
[sahlberg@rawhide-2 git]$ git add test
[sahlberg@rawhide-2 git]$

Note You need to log in before you can comment on or make changes to this bug.