| Message ID | c96f9f4d14c7a40913a39856ca8f065d37113372.1485311744.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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(-)
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
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
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);