Re: [PATCH] Btrfs: disable xattr operations on subvolume directories

Re: [PATCH] Btrfs: disable xattr operations on subvolume directories

[Omar Sandoval]

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.

Re: [PATCH] Btrfs: disable xattr operations on subvolume directories

[Andreas Gruenbacher]

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

Re: [PATCH] Btrfs: disable xattr operations on subvolume directories

[Omar Sandoval]

On Wed, Jan 25, 2017 at 01:28:21PM +0100, Andreas Gruenbacher wrote:
> 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.

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...

> 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?

Yeah, those shouldn't be there, either. I'll get rid of them in v2.

On Wed, Jan 25, 2017 at 01:29:51PM +0100, Andreas Gruenbacher wrote:
> On Wed, Jan 25, 2017 at 3:38 AM, Omar Sandoval <osandov@osandov.com> wrote:
> > 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.

[PATCH] Btrfs: disable xattr operations on subvolume directories

[Omar Sandoval]

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

[PATCH v2 0/3] Btrfs: disable xattr operations on subvolume directories

[Omar Sandoval]

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(-)

-- 
2.11.0

[PATCH v2 1/3] Btrfs: remove old tree_root case in btrfs_read_locked_inode()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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.

Cc: <stable@vger.kernel.org> # 4.9.x
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/inode.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

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

[PATCH v2 2/3] Btrfs: disable xattr operations on subvolume directories

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

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.

Fixes: 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")
Cc: Andreas Gruenbacher <agruenba@redhat.com>
Reported-by: Chris Murphy <lists@colorremedies.com>
Tested-by: Chris Murphy <lists@colorremedies.com>
Cc: <stable@vger.kernel.org> # 4.9.x
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ae3989c22718..924f3583a04d 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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);
-- 
2.11.0

[PATCH v2 3/3] Btrfs: remove ->{get,set}_acl() from btrfs_dir_ro_inode_operations

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Subvolume directory inodes can't have ACLs.

Cc: <stable@vger.kernel.org> # 4.9.x
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 924f3583a04d..1b21a63ed328 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -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,
 };
 
-- 
2.11.0

Re: [PATCH] Btrfs: disable xattr operations on subvolume directories

[Omar Sandoval]

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).

Re: [PATCH v2 3/3] Btrfs: remove ->{get,set}_acl() from btrfs_dir_ro_inode_operations

[David Sterba]

On Wed, Jan 25, 2017 at 05:06:40PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Subvolume directory inodes can't have ACLs.
> 
> Cc: <stable@vger.kernel.org> # 4.9.x
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2 2/3] Btrfs: disable xattr operations on subvolume directories

[David Sterba]

On Wed, Jan 25, 2017 at 05:06:39PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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.
> 
> Fixes: 6c6ef9f26e59 ("xattr: Stop calling {get,set,remove}xattr inode operations")
> Cc: Andreas Gruenbacher <agruenba@redhat.com>
> Reported-by: Chris Murphy <lists@colorremedies.com>
> Tested-by: Chris Murphy <lists@colorremedies.com>
> Cc: <stable@vger.kernel.org> # 4.9.x
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2 1/3] Btrfs: remove old tree_root case in btrfs_read_locked_inode()

[David Sterba]

On Wed, Jan 25, 2017 at 05:06:38PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> 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.
> 
> Cc: <stable@vger.kernel.org> # 4.9.x
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH v2 0/3] Btrfs: disable xattr operations on subvolume directories

[Chris Mason]

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