From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5F63DC433E0 for ; Tue, 16 Feb 2021 19:29:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 258B064ED6 for ; Tue, 16 Feb 2021 19:29:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229880AbhBPT3B (ORCPT ); Tue, 16 Feb 2021 14:29:01 -0500 Received: from mail-ed1-f49.google.com ([209.85.208.49]:40823 "EHLO mail-ed1-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229742AbhBPT3A (ORCPT ); Tue, 16 Feb 2021 14:29:00 -0500 Received: by mail-ed1-f49.google.com with SMTP id q10so13672788edt.7; Tue, 16 Feb 2021 11:28:42 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ODZXxkvQj8tytfBg+T2aqeaBviBhtjaFhAxWsg5ufxg=; b=icDA8ymrCQBT2oFzBKdTyaLLb10xkGyQsBqrMuLlvAWMSHprvKbIFXkR562DycBkIl qjmCOHPZB51otJQzXW6/K2b/jAbnHQ1baPf80qCaz1QEtAzO1rKXU2z1j0EEG8d296l0 +rGg4AMWsGywW6mKAiT10W2jehSuRT6F9H+VtiYKKWJ+zAbLYVaAhWomDQXjp4Ksnfc5 jgfdzHagvjhll0JnFwcBWohjcBl5kg3oLu2h8nJ6PgLr/fDrMk5XNFMjz1Mpmv3N7NAU cK20LgU90eDL4usMpN/UGgS8z5kah38DZRPr5W8Rkl6WuE4P3eBAJbi4WyQU0xCqjDry CcxQ== X-Gm-Message-State: AOAM5328RqgfJ4qzbQnCQ+l4R8Wv/sqV8PF0zF8faGDg2Yq81EIio+cj ctm9yhaKY9aRVsRQwmtFFVh00Ip6y9Hngsc+D+Y= X-Google-Smtp-Source: ABdhPJzQEROv5XSoKZINmGaYhj/FOOGbkolTzgTfy5/fTz7XArBdf6B5/S+YIZ7xeP0yLnmwOJRh2q5yZJrWLTApZ7w= X-Received: by 2002:a05:6402:14c9:: with SMTP id f9mr23073431edx.90.1613503696698; Tue, 16 Feb 2021 11:28:16 -0800 (PST) MIME-Version: 1.0 References: <20210215154317.8590-1-lhenriques@suse.de> <73ab4951f48d69f0183548c7a82f7ae37e286d1c.camel@hammerspace.com> <92d27397479984b95883197d90318ee76995b42e.camel@hammerspace.com> <87r1lgjm7l.fsf@suse.de> <87blckj75z.fsf@suse.de> <874kibkflh.fsf@suse.de> In-Reply-To: From: Anna Schumaker Date: Tue, 16 Feb 2021 14:27:59 -0500 Message-ID: Subject: Re: [PATCH v2] vfs: prevent copy_file_range to copy across devices To: Amir Goldstein Cc: Luis Henriques , Trond Myklebust , "samba-technical@lists.samba.org" , "drinkcat@chromium.org" , "iant@google.com" , "linux-cifs@vger.kernel.org" , "darrick.wong@oracle.com" , "linux-kernel@vger.kernel.org" , "jlayton@kernel.org" , "llozano@chromium.org" , "linux-nfs@vger.kernel.org" , "miklos@szeredi.hu" , "viro@zeniv.linux.org.uk" , "dchinner@redhat.com" , "linux-fsdevel@vger.kernel.org" , "gregkh@linuxfoundation.org" , "sfrench@samba.org" , "ceph-devel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org On Tue, Feb 16, 2021 at 2:22 PM Amir Goldstein wrote: > > On Tue, Feb 16, 2021 at 8:54 PM Luis Henriques wrote: > > > > Amir Goldstein writes: > > > > > On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques wrote: > > >> > > >> Amir Goldstein writes: > > >> > > >> >> Ugh. And I guess overlayfs may have a similar problem. > > >> > > > >> > Not exactly. > > >> > Generally speaking, overlayfs should call vfs_copy_file_range() > > >> > with the flags it got from layer above, so if called from nfsd it > > >> > will allow cross fs copy and when called from syscall it won't. > > >> > > > >> > There are some corner cases where overlayfs could benefit from > > >> > COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but > > >> > let's leave those for now. Just leave overlayfs code as is. > > >> > > >> Got it, thanks for clarifying. > > >> > > >> >> > This is easy to solve with a flag COPY_FILE_SPLICE (or something) that > > >> >> > is internal to kernel users. > > >> >> > > > >> >> > FWIW, you may want to look at the loop in ovl_copy_up_data() > > >> >> > for improvements to nfsd_copy_file_range(). > > >> >> > > > >> >> > We can move the check out to copy_file_range syscall: > > >> >> > > > >> >> > if (flags != 0) > > >> >> > return -EINVAL; > > >> >> > > > >> >> > Leave the fallback from all filesystems and check for the > > >> >> > COPY_FILE_SPLICE flag inside generic_copy_file_range(). > > >> >> > > >> >> Ok, the diff bellow is just to make sure I understood your suggestion. > > >> >> > > >> >> The patch will also need to: > > >> >> > > >> >> - change nfs and overlayfs calls to vfs_copy_file_range() so that they > > >> >> use the new flag. > > >> >> > > >> >> - check flags in generic_copy_file_checks() to make sure only valid flags > > >> >> are used (COPY_FILE_SPLICE at the moment). > > >> >> > > >> >> Also, where should this flag be defined? include/uapi/linux/fs.h? > > >> > > > >> > Grep for REMAP_FILE_ > > >> > Same header file, same Documentation rst file. > > >> > > > >> >> > > >> >> Cheers, > > >> >> -- > > >> >> Luis > > >> >> > > >> >> diff --git a/fs/read_write.c b/fs/read_write.c > > >> >> index 75f764b43418..341d315d2a96 100644 > > >> >> --- a/fs/read_write.c > > >> >> +++ b/fs/read_write.c > > >> >> @@ -1383,6 +1383,13 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in, > > >> >> struct file *file_out, loff_t pos_out, > > >> >> size_t len, unsigned int flags) > > >> >> { > > >> >> + if (!(flags & COPY_FILE_SPLICE)) { > > >> >> + if (!file_out->f_op->copy_file_range) > > >> >> + return -EOPNOTSUPP; > > >> >> + else if (file_out->f_op->copy_file_range != > > >> >> + file_in->f_op->copy_file_range) > > >> >> + return -EXDEV; > > >> >> + } > > >> > > > >> > That looks strange, because you are duplicating the logic in > > >> > do_copy_file_range(). Maybe better: > > >> > > > >> > if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE)) > > >> > return -EINVAL; > > >> > if (flags & COPY_FILE_SPLICE) > > >> > return do_splice_direct(file_in, &pos_in, file_out, &pos_out, > > >> > len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0); > > >> > > >> My initial reasoning for duplicating the logic in do_copy_file_range() was > > >> to allow the generic_copy_file_range() callers to be left unmodified and > > >> allow the filesystems to default to this implementation. > > >> > > >> With this change, I guess that the calls to generic_copy_file_range() from > > >> the different filesystems can be dropped, as in my initial patch, as they > > >> will always get -EINVAL. The other option would be to set the > > >> COPY_FILE_SPLICE flag in those calls, but that would get us back to the > > >> problem we're trying to solve. > > > > > > I don't understand the problem. > > > > > > What exactly is wrong with the code I suggested? > > > Why should any filesystem be changed? > > > > > > Maybe I am missing something. > > > > Ok, I have to do a full brain reboot and start all over. > > > > Before that, I picked the code you suggested and tested it. I've mounted > > a cephfs filesystem and used xfs_io to execute a 'copy_range' command > > using /sys/kernel/debug/sched_features as source. The result was a > > 0-sized file in cephfs. And the reason is thevfs_copy_file_range() > > early exit in: > > > > if (len == 0) > > return 0; > > > > 'len' is set in generic_copy_file_checks(). > > Good point.. I guess we will need to do all the checks earlier in > generic_copy_file_checks() including the logic of: > > if (file_in->f_op->remap_file_range && > file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) > > > > > > This means that we're not solving the original problem anymore (probably > > since v1 of this patch, haven't checked). > > > > Also, re-reading Trond's emails, I read: "... also disallowing the copy > > from, say, an XFS formatted partition to an ext4 partition". Isn't that > > *exactly* what we're trying to do here? I.e. _prevent_ these copies from > > happening so that tracefs files can't be CFR'ed? > > > > We want to address the report which means calls coming from > copy_file_range() syscall. > > Trond's use case is vfs_copy_file_range() coming from nfsd. > When he writes about copy from XFS to ext4, he means an > NFS client is issuing server side copy (on same or different NFS mounts) > and the NFS server is executing nfsd_copy_file_range() on a source > file that happens to be on XFS and destination happens to be on ext4. NFS also supports a server-to-server copy where the destination server mounts the source server and reads the data to be copied. Please don't break that either :) Anna > > We can undo the copy_file_range() syscall change of behavior from > v5.3 without regressing the NFS use case. > > We just need to be careful and look at all the affected code paths. > > Thanks, > Amir.