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

[Omar Sandoval <osandov@osandov.com>]

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.