Re: crash zfs_clone_range()

On Tue, Nov 14, 2023 at 1:15 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On 11/14/23, Rick Macklem <rick.macklem@gmail.com> wrote:
> > On Tue, Nov 14, 2023 at 10:46 AM Alexander Motin <mav@freebsd.org> wrote:
> >>
> >> On 14.11.2023 12:44, Alexander Motin 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);
> >>
> >> Thinking again, what happen if there are two nullfs mounts on top of two
> >> different file systems, one of which is indeed not ZFS?  Do we need to
> >> add those checks to all ZFS, NFS and FUSE, implementing
> >> VOP_COPY_FILE_RANGE, or it is responsibility of nullfs or VFS?
> > Although it would be nice to do the check before the VOP call, I don't
> > see an easy way to do that.
> >
> > It looks like the simple solution is to add a check in each of the
> > VOP_COPY_FILE_RANGE() calls, such as mjg@ has proposed
> > for ZFS. At this point there is only the three and I can easily do the
> > NFS one.
> >
>
> All filesystems except for zfs are already covered because they check
> for mismatched mount.
Yes, now that the mount point(s) are busied. The NFS check is before
the vnodes were locked, so it is unsafe without busying the mount points.
(That was not my patch, but I missed the problem during review.)

rick

>
> --
> Mateusz Guzik <mjguzik gmail.com>