Re: [PATCH] ceph: copy_file_range needs to strip setuid bits and update timestamps

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2019-06-13 at 16:50 +0100, Luis Henriques wrote:
> Jeff Layton <jlayton@kernel.org> writes:
> 
> > On Mon, 2019-06-10 at 20:40 +0300, Amir Goldstein wrote:
> > > Because ceph doesn't hold destination inode lock throughout the copy,
> > > strip setuid bits before and after copy.
> > > 
> > > The destination inode mtime is updated before and after the copy and the
> > > source inode atime is updated after the copy, similar to the filesystem
> > > ->read_iter() implementation.
> > > 
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > > 
> > > Hi Ilya,
> > > 
> > > Please consider applying this patch to ceph branch after merging
> > > Darrick's copy-file-range-fixes branch from:
> > >         git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git
> > > 
> > > The series (including this patch) was tested on ceph by
> > > Luis Henriques using new copy_range xfstests.
> > > 
> > > AFAIK, only fallback from ceph to generic_copy_file_range()
> > > implementation was tested and not the actual ceph clustered
> > > copy_file_range.
> > > 
> > > Thanks,
> > > Amir.
> > > 
> > >  fs/ceph/file.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > > index c5517ffeb11c..b04c97c7d393 100644
> > > --- a/fs/ceph/file.c
> > > +++ b/fs/ceph/file.c
> > > @@ -1949,6 +1949,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> > >  		goto out;
> > >  	}
> > >  
> > > +	/* Should dst_inode lock be held throughout the copy operation? */
> > > +	inode_lock(dst_inode);
> > > +	ret = file_modified(dst_file);
> > > +	inode_unlock(dst_inode);
> > > +	if (ret < 0) {
> > > +		dout("failed to modify dst file before copy (%zd)\n", ret);
> > > +		goto out;
> > > +	}
> > > +
> > 
> > I don't see anything that guarantees that the mode of the destination
> > file is up to date at this point. file_modified() just ends up checking
> > the mode cached in the inode.
> > 
> > I wonder if we ought to fix get_rd_wr_caps() to also acquire a reference
> > to AUTH_SHARED caps on the destination inode, and then call
> > file_modified() after we get those caps. That would also mean that we
> > wouldn't need to do this a second time after the copy.
> > 
> > The catch is that if we did need to issue a setattr, I'm not sure if
> > we'd need to release those caps first.
> > 
> > Luis, Zheng, thoughts?
> 
> Hmm... I missed that.  IIRC the FILE_WR caps allow to modify some
> metadata (such as timestamps, and file size).  I suppose it doesn't
> allow to cache the mode, does it? 

No, W caps don't guarantee that the mode won't change. You need As or Ax
caps for that.

>  If it does, fixing it would be a
> matter of moving the code a bit further down.  If it doesn't the
> ceph_copy_file_range function already has this problem, as it calls
> file_update_time.  And I wonder if other code paths have this problem
> too.
> 

I think you mean file_remove_privs, but yes...the write codepath has a
similar problem. file_remove_privs is called before acquiring any caps,
so the same thing could happen there too.

It'd be good to fix both places, but taking As cap references in the
write codepath could have performance impact in some cases. OTOH, they
don't change that much, so maybe that's OK.

> Obviously, the chunk below will have the same problem.
> 

Right. If however, we have this code take an As cap reference before
doing the copy, then we can be sure that the mode can't change until we
drop them. That way we wouldn't need the second call.

-- 
Jeff Layton <jlayton@kernel.org>