Btrfs: disable xattr operations on subvolume directories
diff mbox

Message ID c96f9f4d14c7a40913a39856ca8f065d37113372.1485311744.git.osandov@fb.com
State Superseded
Headers show

Commit Message

Omar Sandoval Jan. 25, 2017, 2:38 a.m. UTC
From: Omar Sandoval <osandov@fb.com>

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.

Fixes: 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Reported-by: Chris Murphy <lists@colorremedies.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
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(-)

Comments

Omar Sandoval Jan. 25, 2017, 2:38 a.m. UTC | #1
On Tue, Jan 24, 2017 at 06:38:02PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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.
> 
> Fixes: 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")
> Cc: Andreas Gruenbacher <agruenba@redhat.com>
> Reported-by: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> 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(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 4e024260ad71..3dacf0786428 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -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);
> -- 
> 2.11.0
> 

Forgot to cc stable, but 4.9 needs this.
--
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
Andreas Gruenbacher Jan. 25, 2017, 12:28 p.m. UTC | #2
Omar,

On Wed, Jan 25, 2017 at 3:38 AM, Omar Sandoval <osandov@osandov.com> wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> 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.
>
> Fixes: 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")
> Cc: Andreas Gruenbacher <agruenba@redhat.com>
> Reported-by: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> 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
--
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
Omar Sandoval Jan. 26, 2017, 1:06 a.m. UTC | #3
From: Omar Sandoval <osandov@fb.com>

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(-)
Omar Sandoval Jan. 26, 2017, 1:08 a.m. UTC | #4
On Wed, Jan 25, 2017 at 05:06:36PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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.
> 
> Fixes: 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")
> Cc: Andreas Gruenbacher <agruenba@redhat.com>
> Reported-by: Chris Murphy <lists@colorremedies.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> 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).
--
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
Chris Mason Jan. 26, 2017, 11:45 p.m. UTC | #5
On 01/25/2017 08:06 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> 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
--
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

Patch
diff mbox

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 4e024260ad71..3dacf0786428 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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);