diff mbox

setfacl fix

Message ID CAH2r5msswZg_NvY9qMNknh2FWht1jMgwW1s2umtaBj3mHreDAw@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve French Nov. 16, 2013, 2:53 a.m. UTC
From: Steve French <smfrench@gmail.com>
Date: Fri, 15 Nov 2013 20:41:32 -0600
Subject: [PATCH] [CIFS] setfacl removes part of ACL when setting POSIX ACLs to
 Samba

setfacl over cifs mounts can remove the default ACL when setting the
(non-default part of) the ACL and vice versa (we were leaving at 0
rather than setting to -1 the count field for the unaffected
half of the ACL.  For example notice the setfacl removed
the default ACL in this sequence:

steven@steven-GA-970A-DS3:~/cifs-2.6$ getfacl /mnt/test-dir ; setfacl
-m default:user:test:rwx,user:test:rwx /mnt/test-dir
getfacl: Removing leading '/' from absolute path names
user::rwx
group::r-x
other::r-x
default:user::rwx
default:user:test:rwx
default:group::r-x
default:mask::rwx
default:other::r-x

steven@steven-GA-970A-DS3:~/cifs-2.6$ getfacl /mnt/test-dir
getfacl: Removing leading '/' from absolute path names
user::rwx
user:test:rwx
group::r-x
mask::rwx
other::r-x

CC: Stable <stable@kernel.org>
Signed-off-by: Steve French <smfrench@gmail.com>
Acked-by: Jeremy Allison <jra@samba.org>
---
 fs/cifs/cifssmb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

     }

Comments

Jeremy Allison Nov. 16, 2013, 4:55 a.m. UTC | #1
On Fri, Nov 15, 2013 at 08:53:12PM -0600, Steve French wrote:
> From: Steve French <smfrench@gmail.com>
> Date: Fri, 15 Nov 2013 20:41:32 -0600
> Subject: [PATCH] [CIFS] setfacl removes part of ACL when setting POSIX ACLs to
>  Samba
> 
> setfacl over cifs mounts can remove the default ACL when setting the
> (non-default part of) the ACL and vice versa (we were leaving at 0
> rather than setting to -1 the count field for the unaffected
> half of the ACL.  For example notice the setfacl removed
> the default ACL in this sequence:

Looks good to me. Removing the default acl when
the number of ACE entries is set to zero is by
design in the Samba server, to allow setfacl -k
to work (remove the default ACL).

Jeremy.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig Nov. 16, 2013, 2:55 p.m. UTC | #2
On Fri, Nov 15, 2013 at 08:53:12PM -0600, Steve French wrote:
> From: Steve French <smfrench@gmail.com>
> Date: Fri, 15 Nov 2013 20:41:32 -0600
> Subject: [PATCH] [CIFS] setfacl removes part of ACL when setting POSIX ACLs to
>  Samba
> 
> setfacl over cifs mounts can remove the default ACL when setting the
> (non-default part of) the ACL and vice versa (we were leaving at 0
> rather than setting to -1 the count field for the unaffected
> half of the ACL.  For example notice the setfacl removed
> the default ACL in this sequence:

Can you send a patch to xfstests to make sure we regression test for
this issue on all filesystems?

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
steve Nov. 16, 2013, 3:15 p.m. UTC | #3
> > From: Steve French <smfrench@gmail.com>
> > Date: Fri, 15 Nov 2013 20:41:32 -0600
> > Subject: [PATCH] [CIFS] setfacl removes part of ACL when setting POSIX ACLs to
> >  Samba
> > 
> > setfacl over cifs mounts can remove the default ACL 

Hi
cifs-utils 6.1 Samba 4.1.0

I don't understand the '...over cifs mounts...' bit.

Does it mean that if I have a share mounted on my Linux box and do a
setfacl on a file in the share then the default acl will be removed?

Sorry for a non dev question.

Cheers,
Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steve French Nov. 16, 2013, 9:38 p.m. UTC | #4
Makes sense to add a setfacl/getfacl test to xfstest and was trying to
build updated xfstests and look at what has changed but ran into a
strange error building xfstests and didn't see an obvious answer when
googling for it.  Any idea how to workaround the build failure?

Building src
    [DEP]
    [CC]    dirstress
gcc: error: /lib64/libhandle.so: Too many levels of symbolic links


These are the steps I went through from a fairly clean Fedora 19 64
system before the make failure:

git clone git://oss.sgi.com/xfs/cmds/xfstests
git clone git://oss.sgi.com/xfs/cmds/xfsprogs
sudo yum install uuid-devel e2fsprogs-devel libuuid-devel
libattr-devel libacl-devel
cd xfsprogs
make
sudo make install-qa
cd ../xfstests
./configure
make (which failed with the symlink error above)

On Sat, Nov 16, 2013 at 8:55 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Nov 15, 2013 at 08:53:12PM -0600, Steve French wrote:
>> From: Steve French <smfrench@gmail.com>
>> Date: Fri, 15 Nov 2013 20:41:32 -0600
>> Subject: [PATCH] [CIFS] setfacl removes part of ACL when setting POSIX ACLs to
>>  Samba
>>
>> setfacl over cifs mounts can remove the default ACL when setting the
>> (non-default part of) the ACL and vice versa (we were leaving at 0
>> rather than setting to -1 the count field for the unaffected
>> half of the ACL.  For example notice the setfacl removed
>> the default ACL in this sequence:
>
> Can you send a patch to xfstests to make sure we regression test for
> this issue on all filesystems?
>
Steve French Nov. 16, 2013, 9:55 p.m. UTC | #5
On Sat, Nov 16, 2013 at 9:15 AM, steve <steve@steve-ss.com> wrote:
>
>> > From: Steve French <smfrench@gmail.com>
>> > Date: Fri, 15 Nov 2013 20:41:32 -0600
>> > Subject: [PATCH] [CIFS] setfacl removes part of ACL when setting POSIX ACLs to
>> >  Samba
>> >
>> > setfacl over cifs mounts can remove the default ACL
>
> Hi
> cifs-utils 6.1 Samba 4.1.0
>
> I don't understand the '...over cifs mounts...' bit.
>
> Does it mean that if I have a share mounted on my Linux box and do a
> setfacl on a file in the share then the default acl will be removed?
>
> Sorry for a non dev question.

I was surprised too.  Over a cifs mount (to Samba, as Windows does not
support the POSIX style ACLs) it looks like many users paid attention
to only half the POSIX ACL, adding additional users or groups to the
(non-default) ACL and ignoring the default ACL or vice versa.  A user
reported the problem (with part of the ACL getting removed on setfacl
over a cifs mount) a couple days ago, and I found one older reference
to a similar problem.
Parzer, Peter Nov. 18, 2013, 8:05 a.m. UTC | #6
Hi,

I can show you this bug. A share mounted from a Samba server 3.6.3 (Ubuntu 12.04 Server) on Ubuntu 12.04 with kernel 3.2, cifs 1.76.

$ getfacl test
# file: test
# owner: parzerpeter
# group: domänen-benutzer
user::rwx
group::---
group:kjp-admins:rwx
mask::rwx
other::---
default:user::rwx
default:group::---
default:group:kjp-admins:rwx
default:mask::rwx
default:other::---

$ setfacl -m u:kjptest:rx test

$ getfacl test
# file: test
# owner: parzerpeter
# group: domänen-benutzer
user::rwx
user:kjptest:r-x
group::---
group:kjp-admins:rwx
mask::rwx
other::---

$ setfacl -m d:g:kjp-admins:rwx test

$ getfacl test
# file: test
# owner: parzerpeter
# group: domänen-benutzer
user::rwx
group::---
other::---
default:user::rwx
default:group::---
default:group:kjp-admins:rwx
default:mask::rwx
default:other::---


When changing the ACLs, the defaults are removed, and when changing the defaults the ACLs are removed.

Peter
Christoph Hellwig Nov. 18, 2013, 3:27 p.m. UTC | #7
On Sat, Nov 16, 2013 at 03:38:09PM -0600, Steve French wrote:
> Makes sense to add a setfacl/getfacl test to xfstest and was trying to
> build updated xfstests and look at what has changed but ran into a
> strange error building xfstests and didn't see an obvious answer when
> googling for it.  Any idea how to workaround the build failure?
> 
> Building src
>     [DEP]
>     [CC]    dirstress
> gcc: error: /lib64/libhandle.so: Too many levels of symbolic links
> 
> 
> These are the steps I went through from a fairly clean Fedora 19 64
> system before the make failure:

No idea.  Maybe some of the RedHat people on the xfs list have more
experience with Fedora than I have.

> 
> git clone git://oss.sgi.com/xfs/cmds/xfstests
> git clone git://oss.sgi.com/xfs/cmds/xfsprogs
> sudo yum install uuid-devel e2fsprogs-devel libuuid-devel
> libattr-devel libacl-devel
> cd xfsprogs
> make
> sudo make install-qa
> cd ../xfstests
> ./configure
> make (which failed with the symlink error above)
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner Nov. 18, 2013, 10:44 p.m. UTC | #8
On Mon, Nov 18, 2013 at 07:27:23AM -0800, Christoph Hellwig wrote:
> On Sat, Nov 16, 2013 at 03:38:09PM -0600, Steve French wrote:
> > Makes sense to add a setfacl/getfacl test to xfstest and was trying to
> > build updated xfstests and look at what has changed but ran into a
> > strange error building xfstests and didn't see an obvious answer when
> > googling for it.  Any idea how to workaround the build failure?
> > 
> > Building src
> >     [DEP]
> >     [CC]    dirstress
> > gcc: error: /lib64/libhandle.so: Too many levels of symbolic links

What's the circular link chain?

> > These are the steps I went through from a fairly clean Fedora 19 64
> > system before the make failure:
> 
> No idea.  Maybe some of the RedHat people on the xfs list have more
> experience with Fedora than I have.

Don't look at me - I use Debian for all my upstream stuff.... ;)

> > git clone git://oss.sgi.com/xfs/cmds/xfstests
> > git clone git://oss.sgi.com/xfs/cmds/xfsprogs
> > sudo yum install uuid-devel e2fsprogs-devel libuuid-devel
> > libattr-devel libacl-devel
> > cd xfsprogs
> > make
> > sudo make install-qa
> > cd ../xfstests
> > ./configure
> > make (which failed with the symlink error above)

Having the output of all the steps, especially the xfsprogs
install-qa step is probably going to be necessary to debug this. You
probably want to run "make Q= install-qa" so that it runs verbosely.

Cheers,

Dave.
Eric Sandeen Nov. 20, 2013, 5:19 a.m. UTC | #9
On Nov 18, 2013, at 9:27 AM, Christoph Hellwig <hch@infradead.org> wrote:
> 
>> On Sat, Nov 16, 2013 at 03:38:09PM -0600, Steve French wrote:
>> Makes sense to add a setfacl/getfacl test to xfstest and was trying to
>> build updated xfstests and look at what has changed but ran into a
>> strange error building xfstests and didn't see an obvious answer when
>> googling for it.  Any idea how to workaround the build failure?
>> 
>> Building src
>>    [DEP]
>>    [CC]    dirstress
>> gcc: error: /lib64/libhandle.so: Too many levels of symbolic links
>> 
>> 
>> These are the steps I went through from a fairly clean Fedora 19 64
>> system before the make failure:
> 
> No idea.  Maybe some of the RedHat people on the xfs list have more
> experience with Fedora than I have.
> 
Just FWIW fedora has xfsprogs-devel and xfsprogs-qa-devel rpms which should satisfy xfstests for that part of the deps.  I've not done a qa-install manually on fedora since forever...

Eric--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

From 5e8b4b2934093b1e8ec6e8ef30f110cd57e0dbdf Mon Sep 17 00:00:00 2001
From: Steve French <smfrench@gmail.com>
Date: Fri, 15 Nov 2013 20:41:32 -0600
Subject: [PATCH] [CIFS] setfacl removes part of ACL when setting POSIX ACLs to
 Samba

setfacl over cifs mounts can remove the default ACL when setting the
(non-default part of) the ACL and vice versa (we were leaving at 0
rather than setting to -1 the count field for the unaffected
half of the ACL.  For example notice the setfacl removed
the default ACL in this sequence:

steven@steven-GA-970A-DS3:~/cifs-2.6$ getfacl /mnt/test-dir ; setfacl
-m default:user:test:rwx,user:test:rwx /mnt/test-dir
getfacl: Removing leading '/' from absolute path names
user::rwx
group::r-x
other::r-x
default:user::rwx
default:user:test:rwx
default:group::r-x
default:mask::rwx
default:other::r-x

steven@steven-GA-970A-DS3:~/cifs-2.6$ getfacl /mnt/test-dir
getfacl: Removing leading '/' from absolute path names
user::rwx
user:test:rwx
group::r-x
mask::rwx
other::r-x

CC: Stable <stable@kernel.org>
Signed-off-by: Steve French <smfrench@gmail.com>
Acked-by: Jeremy Allison <jra@samba.org>
---
 fs/cifs/cifssmb.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 93b2947..124aa02 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -3369,11 +3369,13 @@  static __u16 ACL_to_cifs_posix(char *parm_data, const char *pACL,
 		return 0;
 	}
 	cifs_acl->version = cpu_to_le16(1);
-	if (acl_type == ACL_TYPE_ACCESS)
+	if (acl_type == ACL_TYPE_ACCESS) {
 		cifs_acl->access_entry_count = cpu_to_le16(count);
-	else if (acl_type == ACL_TYPE_DEFAULT)
+		cifs_acl->default_entry_count = __constant_cpu_to_le16(0xFFFF);
+	} else if (acl_type == ACL_TYPE_DEFAULT) {
 		cifs_acl->default_entry_count = cpu_to_le16(count);
-	else {
+		cifs_acl->access_entry_count = __constant_cpu_to_le16(0xFFFF);
+	} else {
 		cifs_dbg(FYI, "unknown ACL type %d\n", acl_type);
 		return 0;
 	}
-- 
1.8.3.1