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

[Luis Henriques <lhenriques@suse.com>]

Jeff Layton <jlayton@kernel.org> writes:

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

So, do you think the patch below would be enough?  It's totally
untested, but I wanted to know if that would be acceptable before
running some tests on it.

Cheers,
-- 
Luis

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index c5517ffeb11c..f6b0683dd8dc 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -1949,6 +1949,21 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 		goto out;
 	}
 
+	ret = ceph_do_getattr(dst_inode, CEPH_CAP_AUTH_SHARED, false);
+	if (ret < 0) {
+		dout("failed to get auth caps on dst file (%zd)\n", ret);
+		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;
+	}
+
 	/*
 	 * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other
 	 * clients may have dirty data in their caches.  And OSDs know nothing