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@xxxxxxxxx>
> ---
>
> 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?
> /*
> * 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
> @@ -2099,6 +2108,14 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> out:
> ceph_free_cap_flush(prealloc_cf);
>
> + file_accessed(src_file);
> + /* To be on the safe side, try to remove privs also after copy */
> + inode_lock(dst_inode);
> + err = file_modified(dst_file);
> + inode_unlock(dst_inode);
> + if (err < 0)
> + dout("failed to modify dst file after copy (%d)\n", err);
> +
> return ret;
> }
>
