Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 25 Jul 2019 11:48:39 -0500
From:      Kyle Evans <kevans@freebsd.org>
To:        Shawn Webb <shawn.webb@hardenedbsd.org>
Cc:        Rick Macklem <rmacklem@freebsd.org>, svn-src-head <svn-src-head@freebsd.org>,  svn-src-all <svn-src-all@freebsd.org>, src-committers <src-committers@freebsd.org>
Subject:   Re: svn commit: r350315 - in head/sys: kern sys
Message-ID:  <CACNAnaHVWUGG%2B09wQ1MKAWVeQFKzww8VSMT09CFPJUKX1fWZwQ@mail.gmail.com>
In-Reply-To: <20190725164607.zpa7w2pgrnahaxz4@mutt-hbsd>
References:  <201907250546.x6P5kHWq076756@repo.freebsd.org> <20190725164607.zpa7w2pgrnahaxz4@mutt-hbsd>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jul 25, 2019 at 11:46 AM Shawn Webb <shawn.webb@hardenedbsd.org> wrote:
>
> Hey Rick,
>
> On Thu, Jul 25, 2019 at 05:46:17AM +0000, Rick Macklem wrote:
> > Author: rmacklem
> > Date: Thu Jul 25 05:46:16 2019
> > New Revision: 350315
> > URL: https://svnweb.freebsd.org/changeset/base/350315
> >
> > Log:
> >   Add kernel support for a Linux compatible copy_file_range(2) syscall.
> >
> >   This patch adds support to the kernel for a Linux compatible
> >   copy_file_range(2) syscall and the related VOP_COPY_FILE_RANGE(9).
> >   This syscall/VOP can be used by the NFSv4.2 client to implement the
> >   Copy operation against an NFSv4.2 server to do file copies locally on
> >   the server.
> >   The vn_generic_copy_file_range() function in this patch can be used
> >   by the NFSv4.2 server to implement the Copy operation.
> >   Fuse may also me able to use the VOP_COPY_FILE_RANGE() method.
> >
> >   vn_generic_copy_file_range() attempts to maintain holes in the output
> >   file in the range to be copied, but may fail to do so if the input and
> >   output files are on different file systems with different _PC_MIN_HOLE_SIZE
> >   values.
> >
> >   Separate commits will be done for the generated syscall files and userland
> >   changes. A commit for a compat32 syscall will be done later.
> >
> >   Reviewed by:        kib, asomers (plus comments by brooks, jilles)
> >   Relnotes:   yes
> >   Differential Revision:      https://reviews.freebsd.org/D20584
> >
> > Modified:
> >   head/sys/kern/syscalls.master
> >   head/sys/kern/vfs_default.c
> >   head/sys/kern/vfs_syscalls.c
> >   head/sys/kern/vfs_vnops.c
> >   head/sys/kern/vnode_if.src
> >   head/sys/sys/syscallsubr.h
> >   head/sys/sys/vnode.h
> >
> > Modified: head/sys/kern/syscalls.master
> > ==============================================================================
> > --- head/sys/kern/syscalls.master     Thu Jul 25 03:55:05 2019        (r350314)
> > +++ head/sys/kern/syscalls.master     Thu Jul 25 05:46:16 2019        (r350315)
> > @@ -3175,6 +3175,16 @@
> >                   int flag
> >               );
> >       }
> > +569  AUE_NULL        STD {
> > +             ssize_t copy_file_range(
> > +                 int infd,
> > +                 _Inout_opt_ off_t *inoffp,
> > +                 int outfd,
> > +                 _Inout_opt_ off_t *outoffp,
> > +                 size_t len,
> > +                 unsigned int flags
> > +             );
> > +     }
> >
> >  ; Please copy any additions and changes to the following compatability tables:
> >  ; sys/compat/freebsd32/syscalls.master
> >
> > Modified: head/sys/kern/vfs_default.c
> > ==============================================================================
> > --- head/sys/kern/vfs_default.c       Thu Jul 25 03:55:05 2019        (r350314)
> > +++ head/sys/kern/vfs_default.c       Thu Jul 25 05:46:16 2019        (r350315)
> > @@ -83,6 +83,7 @@ static int  dirent_exists(struct vnode *vp, const char
> >  static int vop_stdis_text(struct vop_is_text_args *ap);
> >  static int vop_stdunset_text(struct vop_unset_text_args *ap);
> >  static int vop_stdadd_writecount(struct vop_add_writecount_args *ap);
> > +static int vop_stdcopy_file_range(struct vop_copy_file_range_args *ap);
> >  static int vop_stdfdatasync(struct vop_fdatasync_args *ap);
> >  static int vop_stdgetpages_async(struct vop_getpages_async_args *ap);
> >
> > @@ -140,6 +141,7 @@ struct vop_vector default_vnodeops = {
> >       .vop_set_text =         vop_stdset_text,
> >       .vop_unset_text =       vop_stdunset_text,
> >       .vop_add_writecount =   vop_stdadd_writecount,
> > +     .vop_copy_file_range =  vop_stdcopy_file_range,
> >  };
> >
> >  /*
> > @@ -1210,6 +1212,17 @@ vfs_stdnosync (mp, waitfor)
> >  {
> >
> >       return (0);
> > +}
> > +
> > +static int
> > +vop_stdcopy_file_range(struct vop_copy_file_range_args *ap)
> > +{
> > +     int error;
> > +
> > +     error = vn_generic_copy_file_range(ap->a_invp, ap->a_inoffp,
> > +         ap->a_outvp, ap->a_outoffp, ap->a_lenp, ap->a_flags, ap->a_incred,
> > +         ap->a_outcred, ap->a_fsizetd);
> > +     return (error);
> >  }
> >
> >  int
> >
> > Modified: head/sys/kern/vfs_syscalls.c
> > ==============================================================================
> > --- head/sys/kern/vfs_syscalls.c      Thu Jul 25 03:55:05 2019        (r350314)
> > +++ head/sys/kern/vfs_syscalls.c      Thu Jul 25 05:46:16 2019        (r350315)
> > @@ -4814,3 +4814,122 @@ sys_posix_fadvise(struct thread *td, struct posix_fadv
> >           uap->advice);
> >       return (kern_posix_error(td, error));
> >  }
> > +
> > +int
> > +kern_copy_file_range(struct thread *td, int infd, off_t *inoffp, int outfd,
> > +    off_t *outoffp, size_t len, unsigned int flags)
> > +{
> > +     struct file *infp, *outfp;
> > +     struct vnode *invp, *outvp;
> > +     int error;
> > +     size_t retlen;
> > +     void *rl_rcookie, *rl_wcookie;
> > +     off_t savinoff, savoutoff;
> > +
> > +     infp = outfp = NULL;
> > +     rl_rcookie = rl_wcookie = NULL;
> > +     savinoff = -1;
> > +     error = 0;
> > +     retlen = 0;
> > +
> > +     if (flags != 0) {
> > +             error = EINVAL;
> > +             goto out;
> > +     }
> > +     if (len > SSIZE_MAX)
> > +             /*
> > +              * Although the len argument is size_t, the return argument
> > +              * is ssize_t (which is signed).  Therefore a size that won't
> > +              * fit in ssize_t can't be returned.
> > +              */
> > +             len = SSIZE_MAX;
> > +
> > +     /* Get the file structures for the file descriptors. */
> > +     error = fget_read(td, infd, &cap_read_rights, &infp);
> > +     if (error != 0)
> > +             goto out;
> > +     error = fget_write(td, outfd, &cap_write_rights, &outfp);
> > +     if (error != 0)
> > +             goto out;
> > +
> > +     /* Set the offset pointers to the correct place. */
> > +     if (inoffp == NULL)
> > +             inoffp = &infp->f_offset;
> > +     if (outoffp == NULL)
> > +             outoffp = &outfp->f_offset;
> > +     savinoff = *inoffp;
> > +     savoutoff = *outoffp;
>
> Should these two lines, saving the old inoffp and outoffp, be moved
> before the two conditionals above?
>

Dereferencing potentially NULL pointers like that seems like a scary
proposition; I think this reads most correctly given the context.



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaHVWUGG%2B09wQ1MKAWVeQFKzww8VSMT09CFPJUKX1fWZwQ>