Btrfs devel - Oct 2013 - [PATCH] Btrfs: fix incorrect inode acl reset

When a directory has a default ACL and a subdirectory is created
under that directory, btrfs_init_acl() is called when the
subdirectory''s inode is created to initialize the inode''s ACL
(inherited from the parent directory) but it was clearing the ACL
from the inode after setting it if posix_acl_create() returned
success, instead of clearing it only if it returned an error.

To reproduce this issue:

$ mkfs.btrfs -f /dev/loop0
$ mount /dev/loop0 /mnt
$ mkdir /mnt/acl
$ setfacl -d --set u::rwx,g::rwx,o::- /mnt/acl
$ getfacl /mnt/acl
user::rwx
group::rwx
other::r-x
default:user::rwx
default:group::rwx
default:other::---

$ mkdir /mnt/acl/dir1
$ getfacl /mnt/acl/dir1
user::rwx
group::rwx
other::---

After unmounting and mounting again the filesystem, fgetacl returned the
expected ACL:

$ umount /mnt/acl
$ mount /dev/loop0 /mnt
$ getfacl /mnt/acl/dir1
user::rwx
group::rwx
other::---
default:user::rwx
default:group::rwx
default:other::---

Meaning that the underlying xattr was persisted.

Reported-by: Giuseppe Fierro <giuseppe@fierro.org>
Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---
 fs/btrfs/acl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index e15d2b0..0890c83 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -229,7 +229,7 @@ int btrfs_init_acl(struct btrfs_trans_handle *trans,
 		if (ret > 0) {
 			/* we need an acl */
 			ret = btrfs_set_acl(trans, inode, acl, ACL_TYPE_ACCESS);
-		} else {
+		} else if (ret < 0) {
 			cache_no_acl(inode);
 		}
 	} else {
-- 
1.7.9.5

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

On Tue, Oct 15, 2013 at 06:44:00PM +0100, Filipe David Borba Manana
wrote:
> When a directory has a default ACL and a subdirectory is created
> under that directory, btrfs_init_acl() is called when the
> subdirectory''s inode is created to initialize the inode''s
ACL
> (inherited from the parent directory) but it was clearing the ACL
> from the inode after setting it if posix_acl_create() returned
> success, instead of clearing it only if it returned an error.
> 
> To reproduce this issue:
> 
> $ mkfs.btrfs -f /dev/loop0
> $ mount /dev/loop0 /mnt
> $ mkdir /mnt/acl
> $ setfacl -d --set u::rwx,g::rwx,o::- /mnt/acl
> $ getfacl /mnt/acl
> user::rwx
> group::rwx
> other::r-x
> default:user::rwx
> default:group::rwx
> default:other::---
> 
> $ mkdir /mnt/acl/dir1
> $ getfacl /mnt/acl/dir1
> user::rwx
> group::rwx
> other::---
> 
> After unmounting and mounting again the filesystem, fgetacl returned the
> expected ACL:
> 
> $ umount /mnt/acl
> $ mount /dev/loop0 /mnt
> $ getfacl /mnt/acl/dir1
> user::rwx
> group::rwx
> other::---
> default:user::rwx
> default:group::rwx
> default:other::---
> 
> Meaning that the underlying xattr was persisted.
> 
Can you put this into an xfstest please?  Thanks,

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

On Tue, Oct 15, 2013 at 7:00 PM, Josef Bacik <jbacik@fusionio.com>
wrote:
> On Tue, Oct 15, 2013 at 06:44:00PM +0100, Filipe David Borba Manana wrote:
>> When a directory has a default ACL and a subdirectory is created
>> under that directory, btrfs_init_acl() is called when the
>> subdirectory''s inode is created to initialize the
inode''s ACL
>> (inherited from the parent directory) but it was clearing the ACL
>> from the inode after setting it if posix_acl_create() returned
>> success, instead of clearing it only if it returned an error.
>>
>> To reproduce this issue:
>>
>> $ mkfs.btrfs -f /dev/loop0
>> $ mount /dev/loop0 /mnt
>> $ mkdir /mnt/acl
>> $ setfacl -d --set u::rwx,g::rwx,o::- /mnt/acl
>> $ getfacl /mnt/acl
>> user::rwx
>> group::rwx
>> other::r-x
>> default:user::rwx
>> default:group::rwx
>> default:other::---
>>
>> $ mkdir /mnt/acl/dir1
>> $ getfacl /mnt/acl/dir1
>> user::rwx
>> group::rwx
>> other::---
>>
>> After unmounting and mounting again the filesystem, fgetacl returned
the
>> expected ACL:
>>
>> $ umount /mnt/acl
>> $ mount /dev/loop0 /mnt
>> $ getfacl /mnt/acl/dir1
>> user::rwx
>> group::rwx
>> other::---
>> default:user::rwx
>> default:group::rwx
>> default:other::---
>>
>> Meaning that the underlying xattr was persisted.
>>
>
> Can you put this into an xfstest please?  Thanks,
A new one or just add it to an existing one (and which)?

>
> Josef


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Oct 15, 2013 at 07:02:47PM +0100, Filipe David Manana
wrote:
> > Can you put this into an xfstest please?  Thanks,
> 
> A new one or just add it to an existing one (and which)?
There''s xfstests/shared/051 that tests ACLs, but is run only against
xfs
and udf. I haven''t verified it wrt btrfs, but maybe this test catches
the bug. If not, please add a new test into the ''generic''
category.

thanks,
david
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Oct 15, 2013 at 10:58 PM, David Sterba <dsterba@suse.cz>
wrote:
> On Tue, Oct 15, 2013 at 07:02:47PM +0100, Filipe David Manana wrote:
>> > Can you put this into an xfstest please?  Thanks,
>>
>> A new one or just add it to an existing one (and which)?
>
> There''s xfstests/shared/051 that tests ACLs, but is run only
against xfs
> and udf. I haven''t verified it wrt btrfs, but maybe this test
catches
> the bug. If not, please add a new test into the ''generic''
category.
I adapted shared/051 to make it work with btrfs too.
However the test as it is, doesn''t trigger the default ACL inheritance
issue, so I added the steps from the commit message to the test.

Thanks David.
>
> thanks,
> david


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html