linux-kernel - Re: [PATCH] ceph: fix copy_file_range error path in short copies

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: <CAOi1vP8w_ssGZJTimgDMULgd4jyb_CYuxNyjvHhbBR9FgAqB9A@mail.gmail.com>
Date:   Wed, 5 Feb 2020 12:16:02 +0100
From:   Ilya Dryomov <idryomov@...il.com>
To:     Luis Henriques <lhenriques@...e.com>
Cc:     Jeff Layton <jlayton@...nel.org>, Sage Weil <sage@...hat.com>,
        "Yan, Zheng" <zyan@...hat.com>,
        Gregory Farnum <gfarnum@...hat.com>,
        Ceph Development <ceph-devel@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, stable@...r.kernel.org
Subject: Re: [PATCH] ceph: fix copy_file_range error path in short copies

On Wed, Feb 5, 2020 at 11:28 AM Luis Henriques <lhenriques@...e.com> wrote:
>
> When there's an error in the copying loop but some bytes have already been
> copied into the destination file, it is necessary to dirty the caps and
> eventually update the MDS with the file metadata (timestamps, size).  This
> patch fixes this error path.
>
> Cc: stable@...r.kernel.org
> Signed-off-by: Luis Henriques <lhenriques@...e.com>
> ---
>  fs/ceph/file.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 11929d2bb594..7be47d24edb1 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -2104,9 +2104,16 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>                         CEPH_OSD_OP_FLAG_FADVISE_DONTNEED, 0);
>                 if (err) {
>                         dout("ceph_osdc_copy_from returned %d\n", err);
> -                       if (!ret)
> +                       /*
> +                        * If we haven't done any copy yet, just exit with the
> +                        * error code; otherwise, return the number of bytes
> +                        * already copied, update metadata and dirty caps.
> +                        */
> +                       if (!ret) {
>                                 ret = err;
> -                       goto out_caps;
> +                               goto out_caps;
> +                       }
> +                       goto out_early;
>                 }
>                 len -= object_size;
>                 src_off += object_size;
> @@ -2118,6 +2125,7 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>                 /* We still need one final local copy */
>                 do_final_copy = true;
>
> +out_early:

out_early is misleading, especially given that there already
is out_caps, which just puts caps.  I suggest something like
update_dst_inode.

>         file_update_time(dst_file);
>         inode_inc_iversion_raw(dst_inode);
>

I think this is still buggy.  What follows is this:

        if (endoff > size) {
                int caps_flags = 0;

                /* Let the MDS know about dst file size change */
                if (ceph_quota_is_max_bytes_approaching(dst_inode, endoff))
                        caps_flags |= CHECK_CAPS_NODELAY;
                if (ceph_inode_set_size(dst_inode, endoff))
                        caps_flags |= CHECK_CAPS_AUTHONLY;
                if (caps_flags)
                        ceph_check_caps(dst_ci, caps_flags, NULL);
        }

with endoff being:

        size = i_size_read(dst_inode);
        endoff = dst_off + len;

So a short copy effectively zero-fills the destination file...

Thanks,

                Ilya