From nobody Tue Nov 14 21:25:40 2023 X-Original-To: current@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4SVK8z0Tymz50v1d for ; Tue, 14 Nov 2023 21:25:55 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4SVK8x0n51z3L4D; Tue, 14 Nov 2023 21:25:53 +0000 (UTC) (envelope-from rick.macklem@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pf1-x434.google.com with SMTP id d2e1a72fcca58-6c4d06b6ddaso4245494b3a.3; Tue, 14 Nov 2023 13:25:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1699997151; x=1700601951; darn=freebsd.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=72mr8112Mv6S9k5sb6qCalrEI5j9TALC8lqXQPRB0xY=; b=ISOhmbp4hAjYjWpn4kIWog3wuAGCYtYWvB//ziy7XGMi0EuH8vh5PYhihOA2dhr15T 5cVEmiiwM9QT6jHmTIhKScEsN78Jz3onrXS02UVvbylhnsdMv8s9ujSdGzvzwfPUzf8R lRQfsdkicvy72K6ZUX3WiFs/QxAsH/5Kg9BoC4SIYfZ+hbjUsJ+uFYW/SNgQhBjtVoSP 6xE8Jf85N7FS9tc0/9DEJlngtJ3NuJcOS3XUVUtTKHW5GA1wPoUii8MMGqtiJmo1s/WS 5Zhk6OuTyb7Aacm3A2RUOFkMcq6214I5nMmqMLUxK/5v9Y7rhJajPsDMCTdhGDs1iolC NaDw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699997151; x=1700601951; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=72mr8112Mv6S9k5sb6qCalrEI5j9TALC8lqXQPRB0xY=; b=eqlO2o1XCWbcySVB5TXZsemE9uIm50Nl5qR+WufTkhA4jtX0H4yl3US4whdXU+1MZc R7d5AmT8MmaljP5HRmBbvKi89SclsPr6ZW6COS73IHUo8vrvIW8DMhkgTVFpCRKsWcxD OLr9AVDUPxPhrZznw0KFJUwisCh5LU/CTlDWq8xKzFFIe1hFB7OxtZl985jhMHRLVtAY kkK0qwW76ivGUqceRLADNsLNe3slgArUynDoDkagfwi0pYcXe9zIckmJhQd4eZ/0WFSv aJAFYtkNE8Q8E2wZdUK61J49senmQYqkPJi2dSLcnIJ8kE+c6EssjqYYoDmsqK2ZRmRC x7/w== X-Gm-Message-State: AOJu0YzROvZlvPFMGTkrbV+GEnZh/CUDoOeMvn5zFEElmZ3HWF/Ov94w Lm3L8REtZ6+Qs73X72SSp7a2bR9PllehB9+DGA== X-Google-Smtp-Source: AGHT+IHlBQGTNGQZY9YYsYsvbbKjkEOthfIJ5SG5pt4ru7wOjoThYw382tdVV4UIpSJlJc9PsjjZ4/2NbIlvfAa6pIM= X-Received: by 2002:a05:6a20:c901:b0:186:603b:6b4a with SMTP id gx1-20020a056a20c90100b00186603b6b4amr7228555pzb.32.1699997151134; Tue, 14 Nov 2023 13:25:51 -0800 (PST) List-Id: Discussions about the use of FreeBSD-current List-Archive: https://lists.freebsd.org/archives/freebsd-current List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-current@freebsd.org MIME-Version: 1.0 References: <349700057.3452.1699611152405@localhost> <1900239445.5968.1699966796547@localhost> In-Reply-To: From: Rick Macklem Date: Tue, 14 Nov 2023 13:25:40 -0800 Message-ID: Subject: Re: crash zfs_clone_range() To: Mateusz Guzik Cc: Alexander Motin , Ronald Klop , Konstantin Belousov , current@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; TAGGED_FROM(0.00)[] X-Rspamd-Queue-Id: 4SVK8x0n51z3L4D On Tue, Nov 14, 2023 at 1:15=E2=80=AFPM Mateusz Guzik w= rote: > > On 11/14/23, Rick Macklem wrote: > > On Tue, Nov 14, 2023 at 10:46=E2=80=AFAM Alexander Motin 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 !=3D outvp->v_mount->mnt_vfc) { > >> >> + goto bad_write_fallback; > >> >> + } > >> >> + > >> >> if (invp =3D=3D outvp) { > >> >> if (vn_lock(outvp, LK_EXCLUSIVE) !=3D 0) { > >> >> goto bad_write_fallback; > >> >> > >> > > >> > vn_copy_file_range() verifies for that: > >> > > >> > /* > >> > * If the two vnodes are for the same file system type, cal= l > >> > * VOP_COPY_FILE_RANGE(), otherwise call > >> > vn_generic_copy_file_range() > >> > * which can handle copies across multiple file system type= s. > >> > */ > >> > *lenp =3D len; > >> > if (inmp =3D=3D outmp || strcmp(inmp->mnt_vfc->vfc_name, > >> > outmp->mnt_vfc->vfc_name) =3D=3D 0) > >> > error =3D VOP_COPY_FILE_RANGE(invp, inoffp, outvp, > >> > outoffp, > >> > lenp, flags, incred, outcred, fsize_td); > >> > else > >> > error =3D 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 t= wo > >> 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