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: <20200205192414.GA27345@suse.com>
Date:   Wed, 5 Feb 2020 19:24:14 +0000
From:   Luis Henriques <lhenriques@...e.com>
To:     Ilya Dryomov <idryomov@...il.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 05, 2020 at 12:16:02PM +0100, Ilya Dryomov wrote:
> 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...

Ah!  What a surprise!  Yet another bug in copy_file_range.  /me hides

I guess that replacing 'endoff' by 'dst_off' in the 'if' statement above
(including the condition itself) should fix it.  But I start to think that
I'm biased and unable to see the most obvious issues with this code :-/

Cheers,
--
Luís