[0/3] io path options + reflink (mild security implications)

Message

Kent Overstreet Nov. 12, 2024, 3:35 a.m. UTC

so, I've been fleshing out various things at the intersection of io path
options + rebalance + reflink, and this is the last little bit

background: bcachefs has io path options that can be set filesystem
wide, or per inode, and when changed rebalance automatically picks them
up and does the right thing

reflink adds a wrinkle, which is that we'd like e.g. recursively setting
the foreground/background targets on some files to move them to the
appropriate device (or nr_replicas etc.), like other data - but if a
user did a reflink copy of some other user's data and then set
nr_replicas=1, that would be bad.

so this series adds a flag to reflink pointers for "may propagate option
changes", which can then be set at remap_file_range() time based on
vfs level permission checks.

so, question for everyone: is write access to the source file what we
want? or should it be stricter, i.e. ownership matches?

then, we're currently missing mnt_idmap plumbing to remap_file_range()
to do said permissions checks - do we want to do that? or is there an
easier way?

Kent Overstreet (3):
  bcachefs: BCH_SB_VERSION_INCOMPAT
  bcachefs: bcachefs_metadata_version_reflink_p_may_update_opts
  bcachefs: Option changes now get propagated to reflinked data

 fs/bcachefs/bcachefs.h        |  2 ++
 fs/bcachefs/bcachefs_format.h | 28 +++++++++++--------
 fs/bcachefs/fs-io.c           |  9 ++++++-
 fs/bcachefs/move.c            | 51 ++++++++++++++++++++++++++++++-----
 fs/bcachefs/recovery.c        | 27 ++++++++++++++++---
 fs/bcachefs/reflink.c         | 18 ++++++++++---
 fs/bcachefs/reflink.h         |  3 ++-
 fs/bcachefs/reflink_format.h  |  2 ++
 fs/bcachefs/super-io.c        | 48 ++++++++++++++++++++++++++++++---
 fs/bcachefs/super-io.h        | 18 ++++++++++---
 10 files changed, 172 insertions(+), 34 deletions(-)

Comments

Jan Kara Nov. 12, 2024, 10:53 a.m. UTC | #1

On Mon 11-11-24 22:35:32, Kent Overstreet wrote:
> so, I've been fleshing out various things at the intersection of io path
> options + rebalance + reflink, and this is the last little bit
> 
> background: bcachefs has io path options that can be set filesystem
> wide, or per inode, and when changed rebalance automatically picks them
> up and does the right thing
> 
> reflink adds a wrinkle, which is that we'd like e.g. recursively setting
> the foreground/background targets on some files to move them to the
> appropriate device (or nr_replicas etc.), like other data - but if a
> user did a reflink copy of some other user's data and then set
> nr_replicas=1, that would be bad.
> 
> so this series adds a flag to reflink pointers for "may propagate option
> changes", which can then be set at remap_file_range() time based on
> vfs level permission checks.
> 
> so, question for everyone: is write access to the source file what we
> want? or should it be stricter, i.e. ownership matches?

Well, if I understand the impact properly, this seems similar to the
effects vfs_fileattr_set() can have on a file and there we have:

        if (!inode_owner_or_capable(idmap, inode))
                return -EPERM;

So I'd say ownership match would be more consistent.

> then, we're currently missing mnt_idmap plumbing to remap_file_range()
> to do said permissions checks - do we want to do that? or is there an
> easier way?

Well, if you have struct file, you have the mount and thus idmap through
file_mnt_idmap(file). And struct file is available in remap_file_range()
call stack so I'm not sure what is the problem exactly.

								Honza