Re: crash zfs_clone_range()

On Tue, Nov 14, 2023 at 1:20 PM Konstantin Belousov <kostikbel@gmail.com> wrote:
>
> On Tue, Nov 14, 2023 at 06:47:46PM +0100, Mateusz Guzik wrote:
> > On 11/14/23, Alexander Motin <mav@freebsd.org> wrote:
> > > On 14.11.2023 12:39, Mateusz Guzik wrote:
> > >> One of the vnodes is probably not zfs, I suspect this will do it
> > >> (untested):
> > >>
> > >> diff --git a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > >> b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > >> index 107cd69c756c..e799a7091b8e 100644
> > >> --- a/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > >> +++ b/sys/contrib/openzfs/module/os/freebsd/zfs/zfs_vnops_os.c
> > >> @@ -6270,6 +6270,11 @@ zfs_freebsd_copy_file_range(struct
> > >> vop_copy_file_range_args *ap)
> > >>                          goto bad_write_fallback;
> > >>                  }
> > >>          }
> > >> +
> > >> +       if (invp->v_mount->mnt_vfc != outvp->v_mount->mnt_vfc) {
> > >> +               goto bad_write_fallback;
> > >> +       }
> > >> +
> > >>          if (invp == outvp) {
> > >>                  if (vn_lock(outvp, LK_EXCLUSIVE) != 0) {
> > >>                          goto bad_write_fallback;
> > >>
> > >
> > > vn_copy_file_range() verifies for that:
> > >
> > >          /*
> > >           * If the two vnodes are for the same file system type, call
> > >           * VOP_COPY_FILE_RANGE(), otherwise call
> > > vn_generic_copy_file_range()
> > >           * which can handle copies across multiple file system types.
> > >           */
> > >          *lenp = len;
> > >          if (inmp == outmp || strcmp(inmp->mnt_vfc->vfc_name,
> > >              outmp->mnt_vfc->vfc_name) == 0)
> > >                  error = VOP_COPY_FILE_RANGE(invp, inoffp, outvp, outoffp,
> > >                      lenp, flags, incred, outcred, fsize_td);
> > >          else
> > >                  error = vn_generic_copy_file_range(invp, inoffp, outvp,
> > >                      outoffp, lenp, flags, incred, outcred, fsize_td);
> > >
> > >
> >
> > The crash at hand comes from nullfs. If "outward" vnodes are both
> > nullfs, but only one underlying vnode is zfs, you get the above.
>
> If this is the reason, the check must be done by nullfs bypass for
> vop_copy_file_range().
I suppose this is a reasonable alternative, although it means that
all stacked file systems will need the check.
It just seems easier to do it in the actual VOPs, but it is up to others.

Btw, the stuff above the VOP_COPY_FILE_RANGE() that busies the
mounts and checks mnt_vfc being the same could be dropped, if the
VOP_COPY_FILE_RANGE() calls like NFS were careful to lock the
vnodes before doing a "same fs type or same mount" check.
(I suppose that would be a subtle change in VOP semantics that
 is arguably not allowed for a minor version.)

Anyhow, I am happy with whatever others decide.

rick

>