linux-kernel - Re: [GIT PULL] Btrfs updates for 5.18

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yjo3tQO+fNNlZ4/i@localhost.localdomain>
Date:   Tue, 22 Mar 2022 16:55:17 -0400
From:   Josef Bacik <josef@...icpanda.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     David Sterba <dsterba@...e.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-btrfs <linux-btrfs@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] Btrfs updates for 5.18

On Tue, Mar 22, 2022 at 11:23:21AM -0700, Linus Torvalds wrote:
> On Mon, Mar 21, 2022 at 2:37 PM David Sterba <dsterba@...e.com> wrote:
> >
> > - allow reflinks/deduplication from two different mounts of the same
> >   filesystem
> 
> So I've pulled this, and it looks ok, but I'm not getting the warm and fuzzies.
> 
> In particular, I'm not seeing any commentary about different
> filesystems for this.
> 
> There are several filesystems that use that ->remap_file_range()
> operation, so these relaxed rules don't just affect btrfs.
> 
> Yes, yes, checking for i_sb matching does seem sensible, but I'd
> *really* have liked some sign that people checked with other
> filesystem maintainers and this is ok for all of them, and they didn't
> make assumptions about "always same mount" rather than "always same
> filesystem".
> 

> This affects at least cifs, nfs, overlayfs and ocfs2.

I had a talk with Darrick Wong about this on IRC, and his Reviewed-by is on the
patch.  This did surprise nfsd when xfstests started failing, but talking with
Bruce he didn't complain once he understood what was going on.  Believe me I
have 0 interest in getting the other maintainers upset with me by sneaking
something by them, I made sure to run it by people first, tho I probably should
have checked with people directly other than Darrick.

> 
> Adding fsdevel, and pointing to that
> 
> -       if (src_file->f_path.mnt != dst_file->f_path.mnt)
> +       if (file_inode(src_file)->i_sb != file_inode(dst_file)->i_sb)
> 
> change in commit 9f5710bbfd30 ("fs: allow cross-vfsmount reflink/dedupe")
> 
> And yes, there was already a comment about "Practically, they only
> need to be on the same file system" from before that matches the new
> behavior, but hey, comments have been known to be wrong in the past
> too.
> 
> And yes, I'm also aware that do_clone_file_range() already had that
> exact same i_sb check and it's not new, but since ioctl_file_clone()
> cheched for the mount path, I don't think you could actually reach it
> without being on the same mount.
> 
> And while discussing these sanity checks: wouldn't it make sense to
> check that *both* the source file and the destination file support
> that remap_file_range() op, and it's the same op?
> 
> Yes, yes, it probably always is in practice, but I could imagine some
> type confusion thing. So wouldn't it be nice to also have something
> like
> 
>     if (dst_file->f_op != src_file->f_op)
>           goto out_drop_write;
> 
> in there? I'm thinking "how about dedupe from a directory to a regular
> file" kind of craziness...
>

This more fine-grained checking is handled by generic_remap_file_range_prep() to
make sure we don't try to dedup a directory or pipe or some other nonsense.
Thanks,

Josef