linux-kernel - Re: [RFC PATCH] ceph: fix cross quota realms renames with new truncated files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <925dda9b15044c8a19ac2017d4b135209e1f6184.camel@kernel.org>
Date:   Wed, 11 Nov 2020 18:51:36 -0500
From:   Jeff Layton <jlayton@...nel.org>
To:     Luis Henriques <lhenriques@...e.de>
Cc:     Ilya Dryomov <idryomov@...il.com>, ceph-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Patrick Donnelly <pdonnell@...hat.com>
Subject: Re: [RFC PATCH] ceph: fix cross quota realms renames with new
 truncated files

On Wed, 2020-11-11 at 18:28 +0000, Luis Henriques wrote:
> Jeff Layton <jlayton@...nel.org> writes:
> 
> > On Wed, 2020-11-11 at 15:39 +0000, Luis Henriques wrote:
> > > When doing a rename across quota realms, there's a corner case that isn't
> > > handled correctly.  Here's a testcase:
> > > 
> > >   mkdir files limit
> > >   truncate files/file -s 10G
> > >   setfattr limit -n ceph.quota.max_bytes -v 1000000
> > >   mv files limit/
> > > 
> > > The above will succeed because ftruncate(2) won't result in an immediate
> > > notification of the MDSs with the new file size, and thus the quota realms
> > > stats won't be updated.
> > > 
> > > This patch forces a sync with the MDS every time there's an ATTR_SIZE that
> > > sets a new i_size, even if we have Fx caps.
> > > 
> > > Cc: stable@...r.kernel.org
> > > Fixes: dffdcd71458e ("ceph: allow rename operation under different quota realms")
> > > URL: https://tracker.ceph.com/issues/36593
> > > Signed-off-by: Luis Henriques <lhenriques@...e.de>
> > > ---
> > >  fs/ceph/inode.c | 11 ++---------
> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c
> > > index 526faf4778ce..30e3f240ac96 100644
> > > --- a/fs/ceph/inode.c
> > > +++ b/fs/ceph/inode.c
> > > @@ -2136,15 +2136,8 @@ int __ceph_setattr(struct inode *inode, struct iattr *attr)
> > >  	if (ia_valid & ATTR_SIZE) {
> > >  		dout("setattr %p size %lld -> %lld\n", inode,
> > >  		     inode->i_size, attr->ia_size);
> > > -		if ((issued & CEPH_CAP_FILE_EXCL) &&
> > > -		    attr->ia_size > inode->i_size) {
> > > -			i_size_write(inode, attr->ia_size);
> > > -			inode->i_blocks = calc_inode_blocks(attr->ia_size);
> > > -			ci->i_reported_size = attr->ia_size;
> > > -			dirtied |= CEPH_CAP_FILE_EXCL;
> > > -			ia_valid |= ATTR_MTIME;
> > > -		} else if ((issued & CEPH_CAP_FILE_SHARED) == 0 ||
> > > -			   attr->ia_size != inode->i_size) {
> > > +		if ((issued & (CEPH_CAP_FILE_EXCL|CEPH_CAP_FILE_SHARED)) ||
> > > +		    (attr->ia_size != inode->i_size)) {
> > >  			req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
> > >  			req->r_args.setattr.old_size =
> > >  				cpu_to_le64(inode->i_size);
> > 
> > Hmm...this makes truncates more expensive when we have caps. I'd rather
> > not do that if we can help it.
> 
> Yeah, as I mentioned in the tracker, there's indeed a performance impact
> with this fix.  That's what made me add the RFC in the subject ;-)
> 
> > What about instead having the client mimic a fsync when there is a
> > rename across quota realms? If we can't tell that reliably then we could
> > also just do an effective fsync ahead of any cross-directory rename?
> 
> Ok, thanks for the suggestion.  That may actually work, although it will
> make the rename more expensive of course.  I'll test that tomorrow and
> eventually follow-up with a patch.
> 

Patrick pointed out to me on IRC that since you're moving the parent
directory of the truncated file, flushing the caps on the directory
won't really help. You'd need to walk the entire subtree and try to
flush every dirty inode, or basically do a syncfs() prior to renaming
the directory across quotarealms.

I think we probably will need to revert the change to allow cross-
quotarealm renames of directories and make those return EXDEV again.
Anything else sounds like it's probably going to be too expensive.
-- 
Jeff Layton <jlayton@...nel.org>