| Message ID | 171175869006.1988170.17755870506078239341.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/29] xfs: use unsigned ints for non-negative quantities in xfs_attr_remote.c | expand |
On 2024-03-29 17:43:06, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Create an experimental ioctl so that we can turn off fsverity. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > fs/xfs/libxfs/xfs_fs_staging.h | 3 ++ > fs/xfs/xfs_fsverity.c | 73 ++++++++++++++++++++++++++++++++++++++++ > fs/xfs/xfs_fsverity.h | 3 ++ > fs/xfs/xfs_ioctl.c | 6 +++ > 4 files changed, 85 insertions(+) > > Looks good to me: Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>
On Fri, Mar 29, 2024 at 05:43:06PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Create an experimental ioctl so that we can turn off fsverity. The concept of "experimental ioctls" seems problematic. What if people start relying on them? Linux tends not to have "experimental" system calls, and probably for good reason... Also, what is the use case for this ioctl? Is it necessary to have this when userspace can already just replace a verity file with a copy that has verity disabled? That's less efficient, but it does not require any kernel support and does not require CAP_SYS_ADMIN. And of course, if do we add this ioctl it shouldn't be XFS-specific. - Eric
On Tue, Apr 02, 2024 at 04:25:10PM -0700, Eric Biggers wrote: > On Fri, Mar 29, 2024 at 05:43:06PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Create an experimental ioctl so that we can turn off fsverity. > > The concept of "experimental ioctls" seems problematic. What if people start > relying on them? Linux tends not to have "experimental" system calls, and > probably for good reason... They're trapped in my enormous backlog of patches. They get this special treatment so that I can show them to developers without anyone getting any fancy ideas about merging them. Once I get close enough to actually consider merging it, I'll move it out from under EXPERIMENTAL. IOWs: I'm not planning to push xfs_fs_staging.h itself to upstream ever. > Also, what is the use case for this ioctl? Is it necessary to have this when > userspace can already just replace a verity file with a copy that has verity > disabled? That's less efficient, but it does not require any kernel support and > does not require CAP_SYS_ADMIN. No, of course it isn't needed if replacing the file is easy. That however assumes that replacing /is/ easy. The use case for this is: "I enabled fsverity on my backup volume so I could detect bitrot, then the primary disk died, and when I went to restore the primary, I got a verity error." Being able to read known-bad corrupted contents are less bad than losing the entire file or having to do surgery with xfs_db to turn off fsverity. Just for my own convenience, this would enable me to try out fsverity in a few places while being able to undo it quickly if <cough> we end up changing the ondisk format during review. > And of course, if do we add this ioctl it shouldn't be XFS-specific. Yes, this is a proof of concept. I'd lift it to fs/verity/ if you accept the premise. --D > - Eric >
diff --git a/fs/xfs/libxfs/xfs_fs_staging.h b/fs/xfs/libxfs/xfs_fs_staging.h index 899a56a569d50..4c29167a2b190 100644 --- a/fs/xfs/libxfs/xfs_fs_staging.h +++ b/fs/xfs/libxfs/xfs_fs_staging.h @@ -229,4 +229,7 @@ struct xfs_map_freesp { */ #define XFS_IOC_MAP_FREESP _IOWR('X', 64, struct xfs_map_freesp) +/* Turn off fs-verity */ +#define FS_IOC_DISABLE_VERITY _IO('f', 133) + #endif /* __XFS_FS_STAGING_H__ */ diff --git a/fs/xfs/xfs_fsverity.c b/fs/xfs/xfs_fsverity.c index bfa5c70beec24..f57d8acbd858a 100644 --- a/fs/xfs/xfs_fsverity.c +++ b/fs/xfs/xfs_fsverity.c @@ -792,3 +792,76 @@ const struct fsverity_operations xfs_fsverity_ops = { .drop_merkle_tree_block = xfs_fsverity_drop_merkle, .fail_validation = xfs_fsverity_fail_validation, }; + +/* Turn off fs-verity. */ +int +xfs_fsverity_disable( + struct file *file) +{ + struct inode *inode = file_inode(file); + struct xfs_inode *ip = XFS_I(inode); + struct xfs_mount *mp = ip->i_mount; + struct xfs_trans *tp; + u64 merkle_tree_size; + unsigned int merkle_blocksize; + int error; + + BUILD_BUG_ON(FS_IOC_DISABLE_VERITY == FS_IOC_ENABLE_VERITY); + + if (!xfs_has_verity(mp)) + return -EOPNOTSUPP; + if (!capable(CAP_SYS_ADMIN)) + return -EPERM; + + xfs_ilock(ip, XFS_IOLOCK_EXCL); + + if (!IS_VERITY(inode)) { + error = 0; + goto out_iolock; + } + + if (xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION)) { + error = -EBUSY; + goto out_iolock; + } + + error = xfs_qm_dqattach(ip); + if (error) + goto out_iolock; + + error = fsverity_merkle_tree_geometry(inode, &merkle_blocksize, + &merkle_tree_size); + if (error) + goto out_iolock; + + xfs_fsverity_cache_drop(ip); + + /* Clear fsverity inode flag */ + error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_ichange, 0, 0, false, + &tp); + if (error) + goto out_iolock; + + ip->i_diflags2 &= ~XFS_DIFLAG2_VERITY; + + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); + xfs_trans_set_sync(tp); + + error = xfs_trans_commit(tp); + xfs_iunlock(ip, XFS_ILOCK_EXCL); + if (error) + goto out_iolock; + + inode->i_flags &= ~S_VERITY; + fsverity_cleanup_inode(inode); + + /* Remove the fsverity xattrs. */ + error = xfs_fsverity_delete_metadata(ip, merkle_tree_size, + merkle_blocksize); + if (error) + goto out_iolock; + +out_iolock: + xfs_iunlock(ip, XFS_IOLOCK_EXCL); + return error; +} diff --git a/fs/xfs/xfs_fsverity.h b/fs/xfs/xfs_fsverity.h index 21ba0d82f26d8..4b9fff6b0d2c4 100644 --- a/fs/xfs/xfs_fsverity.h +++ b/fs/xfs/xfs_fsverity.h @@ -17,6 +17,8 @@ struct xfs_icwalk; int xfs_fsverity_scan_inode(struct xfs_inode *ip, struct xfs_icwalk *icw); extern const struct fsverity_operations xfs_fsverity_ops; + +int xfs_fsverity_disable(struct file *file); #else # define xfs_fsverity_cache_init(ip) ((void)0) # define xfs_fsverity_cache_drop(ip) ((void)0) @@ -24,6 +26,7 @@ extern const struct fsverity_operations xfs_fsverity_ops; # define xfs_fsverity_register_shrinker(mp) (0) # define xfs_fsverity_unregister_shrinker(mp) ((void)0) # define xfs_fsverity_scan_inode(ip, icw) (0) +# define xfs_fsverity_disable(ip) (-EOPNOTSUPP) #endif /* CONFIG_FS_VERITY */ #endif /* __XFS_FSVERITY_H__ */ diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 0aa0ceb9ec153..24deaaf5eb0f5 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -44,6 +44,7 @@ #include "xfs_file.h" #include "xfs_exchrange.h" #include "xfs_rtgroup.h" +#include "xfs_fsverity.h" #include <linux/mount.h> #include <linux/namei.h> @@ -2712,6 +2713,11 @@ xfs_file_ioctl( case XFS_IOC_MAP_FREESP: return xfs_ioc_map_freesp(filp, arg); +#ifdef CONFIG_XFS_EXPERIMENTAL_IOCTLS + case FS_IOC_DISABLE_VERITY: + return xfs_fsverity_disable(filp); +#endif + case FS_IOC_ENABLE_VERITY: if (!xfs_has_verity(mp)) return -EOPNOTSUPP;