Re: [PATCH v2] ceph: fix copy_file_range error path in short copies

[Ilya Dryomov]

On Tue, Feb 11, 2020 at 11:32 AM Luis Henriques <lhenriques@xxxxxxxx> wrote:
>
> On Mon, Feb 10, 2020 at 07:38:10PM +0100, Ilya Dryomov wrote:
> > On Thu, Feb 6, 2020 at 11:38 AM Luis Henriques <lhenriques@xxxxxxxx> 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.
> > >
> > > Another issue this patch fixes is the destination file size being reported
> > > to the MDS.  If we're on the error path but the amount of bytes written
> > > has already changed the destination file size, the offset to use is
> > > dst_off and not endoff.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Luis Henriques <lhenriques@xxxxxxxx>
> > > ---
> > >  fs/ceph/file.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > > index 11929d2bb594..f7f8cb6c243f 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 update_dst_inode;
> > >                 }
> > >                 len -= object_size;
> > >                 src_off += object_size;
> > > @@ -2118,16 +2125,17 @@ 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;
> > >
> > > +update_dst_inode:
> > >         file_update_time(dst_file);
> > >         inode_inc_iversion_raw(dst_inode);
> > >
> > > -       if (endoff > size) {
> > > +       if (dst_off > size) {
> > >                 int caps_flags = 0;
> > >
> > >                 /* Let the MDS know about dst file size change */
> > > -               if (ceph_quota_is_max_bytes_approaching(dst_inode, endoff))
> > > +               if (ceph_quota_is_max_bytes_approaching(dst_inode, dst_off))
> > >                         caps_flags |= CHECK_CAPS_NODELAY;
> > > -               if (ceph_inode_set_size(dst_inode, endoff))
> > > +               if (ceph_inode_set_size(dst_inode, dst_off))
> > >                         caps_flags |= CHECK_CAPS_AUTHONLY;
> > >                 if (caps_flags)
> > >                         ceph_check_caps(dst_ci, caps_flags, NULL);
> >
> > Hi Luis,
> >
> > I think this function still has short copy and file size issues:
> >
> > - do_splice_direct() may write fewer bytes than requested, including
> >   nothing at all (i.e. return 0).  While we don't care about the second
> >   call much, handling the first call is crucial because proceeding to
> >   the copy-from loop with src/dst_off not at the object boundary will
> >   corrupt the destination file.
> >
> > - size is set after caps are acquired for the first time and never
> >   updated.  But caps are dropped before do_splice_direct(), so by the
> >   time we get to dst_off > size check, it may be stale.  Again, data
> >   loss if e.g. old-size < dst_off < new-size because the destination
> >   file will get truncated...
> >
> > Also, src/dst_oloc need to be freed with ceph_oloc_destroy() to avoid
> > leaking memory on namespace layouts.
> >
> > It seems clear that this function needs to be split, with the new
> > loop around do_splice_direct() and the copy-from loop each going into
> > a separate functions with clear pre- and post-conditions.
>
> Right, it makes sense to refactor this function and fix all these issues
> you're pointing.  It'll be a pain because a lot of parameters will need to
> be handed over into these new functions (maybe a new 'struct copy_desc'
> can help making it a bit less messy).  Anyway, I'll try to spend some time
> working on that and see what I can come up with.

Yeah, this code really needs more work and extensive verification.

I'm dropping this patch because it's only a partial fix.  Backporting
it alone, with known data corruption issues remaining (and without the
copy-from2 patch that fixes another data corruption issue that is much
easier to hit), doesn't make sense.

Thanks,

                Ilya