Btrfs: disable xattr operations on subvolume directories
Subsystems (2)
PATCH 1/1 Btrfs: disable xattr operations on subvolume directories
From: Omar Sandoval <[email protected]>
When you snapshot a subvolume containing a subvolume, you get a placeholder read-only directory where the subvolume would be. These directory inodes have ->i_ops set to btrfs_dir_ro_inode_operations. Previously, this didn't include the xattr operation callbacks. The conversion to xattr_handlers missed this case, leading to bogus attempts to set xattrs on these inodes. This manifested itself as failures when running delayed inodes.
To fix this, clear the IOP_XATTR in ->i_opflags on these inodes.
---
Applies to v4.10-rc4. Chris, this fixes the issue for me, could you please test it out? Andreas, does this make sense? I'll try to cook up an xfstest for this.
fs/btrfs/inode.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
fs/btrfs/inode.c +5 -2
@@ -3835,10 +3835,12 @@ static int btrfs_read_locked_inode(struct inode *inode)break ;case S_IFDIR : inode->i_fop = &btrfs_dir_file_operations; -if (root == fs_info->tree_root) +if (root == fs_info->tree_root) { inode->i_op = &btrfs_dir_ro_inode_operations; -else + inode->i_opflags &= ~IOP_XATTR ; + }else { inode->i_op = &btrfs_dir_inode_operations; + }break ;case S_IFLNK : inode->i_op = &btrfs_symlink_inode_operations;
@@ -5710,6 +5712,7 @@ static struct inode *new_simple_dir(struct super_block *s, inode->i_ino =BTRFS_EMPTY_SUBVOL_DIR_OBJECTID ; inode->i_op = &btrfs_dir_ro_inode_operations; + inode->i_opflags &= ~IOP_XATTR ; inode->i_fop = &simple_dir_operations; inode->i_mode =S_IFDIR |S_IRUGO |S_IWUSR |S_IXUGO ; inode->i_mtime =current_time (inode);
Signature · 1 line
2.11.0
Quoted 53 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When you snapshot a subvolume containing a subvolume, you get a placeholder read-only directory where the subvolume would be. These directory inodes have ->i_ops set to
btrfs_dir_ro_inode_operations. Previously, this didn't include the xattr operation callbacks. The conversion toxattr_handlersmissed this case, leading to bogus attempts to set xattrs on these inodes. This manifested itself as failures when running delayed inodes.To fix this, clear the
IOP_XATTRin ->i_opflags on these inodes.Fixes 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")Cc Andreas Gruenbacher <[email protected]>Reported-by Chris Murphy <[email protected]>Signed-off-by Omar Sandoval <[email protected]>---
Applies to v4.10-rc4. Chris, this fixes the issue for me, could you please test it out? Andreas, does this make sense? I'll try to cook up an xfstest for this.fs/btrfs/inode.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)fs/btrfs/inode.c +5
-2@@ -3835,10 +3835,12 @@ static int btrfs_read_locked_inode(struct inode *inode)break ;case S_IFDIR : inode->i_fop = &btrfs_dir_file_operations; -if (root == fs_info->tree_root) +if (root == fs_info->tree_root) { inode->i_op = &btrfs_dir_ro_inode_operations; -else + inode->i_opflags &= ~IOP_XATTR ; + }else { inode->i_op = &btrfs_dir_inode_operations; + }break ;case S_IFLNK : inode->i_op = &btrfs_symlink_inode_operations;@@ -5710,6 +5712,7 @@ static struct inode *new_simple_dir(struct super_block *s, inode->i_ino =BTRFS_EMPTY_SUBVOL_DIR_OBJECTID ; inode->i_op = &btrfs_dir_ro_inode_operations; + inode->i_opflags &= ~IOP_XATTR ; inode->i_fop = &simple_dir_operations; inode->i_mode =S_IFDIR |S_IRUGO |S_IWUSR |S_IXUGO ; inode->i_mtime =current_time (inode);Signature · 1 line
2.11.0
Forgot to cc stable, but 4.9 needs this.
Omar,
Quoted 12 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When you snapshot a subvolume containing a subvolume, you get a placeholder read-only directory where the subvolume would be. These directory inodes have ->i_ops set to
btrfs_dir_ro_inode_operations. Previously, this didn't include the xattr operation callbacks. The conversion toxattr_handlersmissed this case, leading to bogus attempts to set xattrs on these inodes. This manifested itself as failures when running delayed inodes.To fix this, clear the
IOP_XATTRin ->i_opflags on these inodes.
Quoted 7 lines
Fixes 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")Cc Andreas Gruenbacher <[email protected]>Reported-by Chris Murphy <[email protected]>Signed-off-by Omar Sandoval <[email protected]>---
Applies to v4.10-rc4. Chris, this fixes the issue for me, could you please test it out? Andreas, does this make sense? I'll try to cook up an xfstest for this.
this change looks good.
Are those directories really read-only though? They have the S_IWUSR permission set, and an update_time iop.
Also, the get_acl and set_acl iops seem dead: they were not called before because the xattr iops were not defined in btrfs_dir_ro_inode_operations, and they are not called now because IOP_XATTR is cleared. Could you please check that as well?
Thanks,
Andreas
Quoted 23 lines · Andreas Gruenbacher
Omar,
Quoted 12 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When you snapshot a subvolume containing a subvolume, you get a placeholder read-only directory where the subvolume would be. These directory inodes have ->i_ops set to
btrfs_dir_ro_inode_operations. Previously, this didn't include the xattr operation callbacks. The conversion toxattr_handlersmissed this case, leading to bogus attempts to set xattrs on these inodes. This manifested itself as failures when running delayed inodes.To fix this, clear the
IOP_XATTRin ->i_opflags on these inodes.Quoted 7 lines
Fixes 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")Cc Andreas Gruenbacher <[email protected]>Reported-by Chris Murphy <[email protected]>Signed-off-by Omar Sandoval <[email protected]>---
Applies to v4.10-rc4. Chris, this fixes the issue for me, could you please test it out? Andreas, does this make sense? I'll try to cook up an xfstest for this.
Quoted 4 lines
this change looks good.
Are those directories really read-only though? They have the
S_IWUSRpermission set, and anupdate_timeiop.
Hm, so these inodes don't have an on-disk [mca]time, it's only in memory. ->update_time() just updates the in-memory time stamp, which is kind of weird. Not sure why these even have S_IWUSR; they don't have a ->create() iop either. 0555 really makes more sense here. I wonder if anyone would care if we made that change...
Quoted 4 lines
Also, the
get_aclandset_acliops seem dead: they were not called before because the xattr iops were not defined inbtrfs_dir_ro_inode_operations, and they are not called now becauseIOP_XATTRis cleared. Could you please check that as well?
Yeah, those shouldn't be there, either. I'll get rid of them in v2.
Quoted 4 lines · Andreas Gruenbacher
Quoted 1 line · Omar Sandoval
Forgot to cc stable, but 4.9 needs this.
Huh, stable not CCed again?
I was hoping Dave would add it when he applied it, but since I'm going to send out a v2, I'll just do it then.
v2 cover Btrfs: disable xattr operations on subvolume directories
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
This series is based on v4.10-rc4. It should probably go in for v4.10 and to stable for v4.9.x.
Changes from v1:
- Added patch 1 to remove an obsolete place where we use btrfs_dir_ro_inode_operations since the fix in patch 2 concerns btrfs_dir_ro_inode_operations.
- Added patch 3. It's mostly a cleanup, but I can't convince myself that there aren't any in-kernel callers that will change ACLs directly. Let's get this in just to be safe.
There are some more followup cleanups to do here, but let's get this fix in first.
Omar Sandoval (3):
Btrfs: remove old tree_root case in btrfs_read_locked_inode()
Btrfs: disable xattr operations on subvolume directories
Btrfs: remove ->{get,set}_acl() from btrfs_dir_ro_inode_operations
fs/btrfs/inode.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Signature · 1 line
2.11.0
Quoted 4 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
This series is based on v4.10-rc4. It should probably go in for v4.10 and to stable for v4.9.x.
Thanks Omar, I've got this queued on top of Dave's pull.
-chris
PATCH v2 1/3 Btrfs: remove old tree_root case in btrfs_read_locked_inode()
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
As Jeff explained in c2951f32d36c ("btrfs: remove old tree_root dirent processing in btrfs_real_readdir()"), supporting this old format is no longer necessary since the Btrfs magic number has been updated since we changed to the current format. There are other places where we still handle this old format, but since this is part of a fix that is going to stable, I'm only removing this one for now.
--- fs/btrfs/inode.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
fs/btrfs/inode.c +1 -4
@@ -3835,10 +3835,7 @@ static int btrfs_read_locked_inode(struct inode *inode)break ;case S_IFDIR : inode->i_fop = &btrfs_dir_file_operations; -if (root == fs_info->tree_root) - inode->i_op = &btrfs_dir_ro_inode_operations; -else - inode->i_op = &btrfs_dir_inode_operations; + inode->i_op = &btrfs_dir_inode_operations;break ;case S_IFLNK : inode->i_op = &btrfs_symlink_inode_operations;
Signature · 1 line
2.11.0
Re: [PATCH v2 1/3] Btrfs: remove old tree_root case in btrfs_read_locked_inode()
Quoted 11 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
As Jeff explained in
c2951f32d36c("btrfs: remove oldtree_rootdirent processing inbtrfs_real_readdir()"), supporting this old format is no longer necessary since the Btrfs magic number has been updated since we changed to the current format. There are other places where we still handle this old format, but since this is part of a fix that is going to stable, I'm only removing this one for now.Cc <[email protected]> # 4.9.xSigned-off-by Omar Sandoval <[email protected]>
PATCH v2 2/3 Btrfs: disable xattr operations on subvolume directories
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
When you snapshot a subvolume containing a subvolume, you get a placeholder directory where the subvolume would be. These directory inodes have ->i_ops set to btrfs_dir_ro_inode_operations. Previously, these i_ops didn't include the xattr operation callbacks. The conversion to xattr_handlers missed this case, leading to bogus attempts to set xattrs on these inodes. This manifested itself as failures when running delayed inodes.
To fix this, clear IOP_XATTR in ->i_opflags on these inodes.
--- fs/btrfs/inode.c | 1 + 1 file changed, 1 insertion(+)
fs/btrfs/inode.c +1 -0
@@ -5707,6 +5707,7 @@ static struct inode *new_simple_dir(struct super_block *s, inode->i_ino =BTRFS_EMPTY_SUBVOL_DIR_OBJECTID ; inode->i_op = &btrfs_dir_ro_inode_operations; + inode->i_opflags &= ~IOP_XATTR ; inode->i_fop = &simple_dir_operations; inode->i_mode =S_IFDIR |S_IRUGO |S_IWUSR |S_IXUGO ; inode->i_mtime =current_time (inode);
Signature · 1 line
2.11.0
Quoted 10 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When you snapshot a subvolume containing a subvolume, you get a placeholder directory where the subvolume would be. These directory inodes have ->i_ops set to
btrfs_dir_ro_inode_operations. Previously, thesei_opsdidn't include the xattr operation callbacks. The conversion toxattr_handlersmissed this case, leading to bogus attempts to set xattrs on these inodes. This manifested itself as failures when running delayed inodes.
Quoted 8 lines
To fix this, clear
IOP_XATTRin ->i_opflags on these inodes.Fixes 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")Cc Andreas Gruenbacher <[email protected]>Reported-by Chris Murphy <[email protected]>Tested-by Chris Murphy <[email protected]>Cc <[email protected]> # 4.9.xSigned-off-by Omar Sandoval <[email protected]>
PATCH v2 3/3 Btrfs: remove ->{get,set}_acl() from btrfs_dir_ro_inode_operations
In reply to Omar SandovalFrom: Omar Sandoval <[email protected]>
Subvolume directory inodes can't have ACLs.
--- fs/btrfs/inode.c | 2 -- 1 file changed, 2 deletions(-)
fs/btrfs/inode.c +0 -2
@@ -10577,8 +10577,6 @@ static const struct inode_operations btrfs_dir_inode_operations = {static const struct inode_operations btrfs_dir_ro_inode_operations = { .lookup = btrfs_lookup, .permission = btrfs_permission, - .get_acl = btrfs_get_acl, - .set_acl = btrfs_set_acl, .update_time = btrfs_update_time, };
Signature · 1 line
2.11.0
Re: [PATCH v2 3/3] Btrfs: remove ->{get,set}_acl() from btrfs_dir_ro_inode_operations
Quoted 6 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
Subvolume directory inodes can't have ACLs.
Cc <[email protected]> # 4.9.xSigned-off-by Omar Sandoval <[email protected]>
Quoted 12 lines · Omar Sandoval
From: Omar Sandoval <[email protected]>
When you snapshot a subvolume containing a subvolume, you get a placeholder read-only directory where the subvolume would be. These directory inodes have ->i_ops set to
btrfs_dir_ro_inode_operations. Previously, this didn't include the xattr operation callbacks. The conversion toxattr_handlersmissed this case, leading to bogus attempts to set xattrs on these inodes. This manifested itself as failures when running delayed inodes.To fix this, clear the
IOP_XATTRin ->i_opflags on these inodes.
Quoted 8 lines
Fixes 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")Cc Andreas Gruenbacher <[email protected]>Reported-by Chris Murphy <[email protected]>Signed-off-by Omar Sandoval <[email protected]>---
Applies to v4.10-rc4. Chris, this fixes the issue for me, could you please test it out? Andreas, does this make sense? I'll try to cook up an xfstest for this.
(Sorry, accidentally resent v1, ignore this one and just look at v2).